Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

P5: Require a gRFC for large scale changes to gRPC core #233

Closed
wants to merge 7 commits into from
Closed
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions P5-core-large-scale-changes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
Title
----
* Author(s): ctiller@google.com (Craig Tiller)
* Approver: a11r
* Status: Draft
* Implemented in: N/A
* Last updated: 4/29/2021
* Discussion at: https://groups.google.com/g/grpc-io/c/q11WFj3KQOs

## Abstract

Assert that gRFC's are needed before large scale changes to gRPC Core are undertaken.
ctiller marked this conversation as resolved.
Show resolved Hide resolved

## Background

The gRFC process was intended to (quoting from README.md in this repository):
* Provide increased visibility to the community on upcoming changes and the design considerations around them.
* Provide the ability to reason about larger "sets" of changes that are too big to be covered either in an Issue or in a PR.
* Establish a consistent process for structured participation by the community on large changes, especially those that impact multiple runtimes and implementations.

There are now multiple teams contributing to gRPC Core, each with different motivations and organizational aims.
These teams form a community that needs to find consensus on overall direction of the gRPC core codebase.
As such, the core API should no longer be considered a cut point for gRFC's, and instead any larger change ought to be considered for such a process.

### Related Proposals:
* This change extends P3 to expect gRFC's for changes to gRPC Core's internals and not just its public API.
ctiller marked this conversation as resolved.
Show resolved Hide resolved

## Proposal

Any large scale change to gRPC Core should not be implemented without following the gRFC process.

A large scale change is one that has a wide-ranging impact on the implementation of gRPC Core.

Such changes include:
* Changes to extensibility points, or the types used by extensibility points.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this bullet in particular needs to be more nuanced. I do absolutely agree that a gRFC is appropriate for large structural changes to these kinds of APIs, but I think we also have a lot of changes to these APIs that are smaller in scope, where requiring a gRFC doesn't seem like a positive cost/benefit trade-off.

In particular, the resolver and LB policy APIs have been seeing a fair amount of small changes over time, some as part of the xDS work and others just as we evolve the client channel architecture, which we've currently been free to do since they are not yet public APIs. It's been very convenient to make these kinds of changes happen slowly over time, and I think that would be a much heavier-weight approach if every small change required a gRFC.

To make this more concerete, here are some examples of actual changes we've made to these APIs over the years, grouped based on whether or not I think the change should require a gRFC moving forward:

First, the changes that I think should require a gRFC:

Here are a few that I think are borderline; the scope or impact of the changes probably justifies a gRFC, but the fact that there don't happen to be any external LB policy implementations today mean that the changes didn't affect anyone:

And here are the ones that I think are small enough that they should not require a gRFC:

(I think there are more in that last category, but I didn't take the time to go back further into the history.)

Given the above, I propose something like the following for these extension points:

  • If the change is a major structural change or fundamental API redesign, it requires a gRFC.
  • If the change affects almost all implementations of an API that is used externally, it requires a gRFC.
  • If the change adds, removes, or modifies a significant part of the API surface (i.e., more than just an individual parameter), it requires a gRFC.
  • Any smaller change not in the above categories does not require a gRFC.

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not opposed, but wanted to throw out an alternative:

This gRFC includes a light-weight update procedure for prior gRFC's that fall into it's domain.
Suppose we retroactively document the interfaces we're worried about here and publish them as gRFC's and then going forward we use the update process (and tune the timing to something that makes sense).

* Changes that require downstream consumers of gRPC Core to modify their code.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this one already covered by P3? If not, can you give an example of what would be included in this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was thinking about internal extensibility... I'll clean this one up.

* Changes with a significant performance impact.
* Changes to vocabulary types - types that are used broadly within the library.
* Changes that constrain future development.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example of this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One that's happened before: L6 introduced the restriction that a C++11 compiler was required to compile Core, L59 required a standard library be dynamically linkable. Neither formally required a gRFC (no API changes), but we did so anyway because it felt right - this tries to formalize that.

New changes that might fall into that: taking a large dependency, refactoring in a way that prevents a known use case... things that would require a large scale change to undo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed text.

* Changes that modify system architecture.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here, an example would help.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some text.


Since implementation experience can affect how a large scale change may proceed, it's additionally proposed that LSC gRFC's may be updated by later PR's against the approved change.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good idea to split this part into a separate P-series gRFC. In practice, we've actually been just sending PRs to update existing gRFCs when we need to, without any real review period at all. If we want to formalize this, then this process should apply to more than just C-core changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah agree.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to not re-trigger the review period :)

Such updates would be subject to a minimum five business day approval process, rather than the usual ten as it's assumed that stake holders would already be identified and have sufficient context.
Updates to LSC gRFC's must add or extend a final section 'Change Log' with a summary of what has changed from version to version of the gRFC.

Finally, since the five day period has been somewhat arbitrarily chosen, as has what is included and excluded - this gRFC may be updated with the same process as for LSC's.

## Rationale

This gRFC will slow down progress on some kinds of changes for gRPC Core.
At this point in gRPC's lifetime this is probably appropriate: gRPC is a widely used product, multiple teams contribute to its development, and any single person cannot cover all of the impact of changes on the myriad use cases for which gRPC is deployed.

By moving the design process to written discussion we lessen the need for meetings.
This helps us include more voices in the design process - people who may not work at Google, or may be intimidated to speak up in large groups.
By identifying design problems before they reach downstream overall project velocity will likely increase.

## Implementation

N/A

## Open issues (if applicable)

N/A