-
Notifications
You must be signed in to change notification settings - Fork 239
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
Update readme and template to reflect actual process being followed #275
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,78 +1,89 @@ | ||
# gRPC RFCs | ||
|
||
## Introduction | ||
Please read the gRPC organization's [governance rules](https://github.com/grpc/grpc-community/blob/master/governance.md) | ||
and [contribution guidelines](https://github.com/grpc/grpc-community/blob/master/CONTRIBUTING.md) before proceeding. | ||
|
||
This repo contains the design proposals for substantial feature changes for | ||
gRPC that need to be designed upfront. The goal of the upfront design process | ||
is to: | ||
- Provide increased visibility to the community on upcoming changes and the design considerations around them. | ||
- Provide 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. | ||
Please read the gRPC organization's [governance | ||
rules](https://github.com/grpc/grpc-community/blob/master/governance.md) and | ||
[contribution | ||
guidelines](https://github.com/grpc/grpc-community/blob/master/CONTRIBUTING.md) | ||
before proceeding. | ||
|
||
This repo contains the design proposals for substantial feature changes for gRPC | ||
that need to be designed upfront. The goal of the upfront design process is to: | ||
|
||
- Provide increased visibility to the community on upcoming changes and the | ||
design considerations around them. | ||
- Provide 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. | ||
|
||
## Prerequisites | ||
This process needs to be followed for any significant change to gRPC that | ||
needs design. | ||
Changes that are considered significant can be: | ||
|
||
This process needs to be followed for any significant change to gRPC that needs | ||
design. Changes that are considered significant can be: | ||
|
||
- Features that need implementation across runtimes and languages. | ||
- Process changes that affect how the gRPC product is implemented. | ||
- Breaking changes to the public API (i.e. semver major changes). | ||
|
||
## Process | ||
|
||
1. Fork the repo and copy the template [GRFC-TEMPLATE.md](GRFC-TEMPLATE.md). | ||
1. Rename it to ``$CategoryName-$Summary``, eg.: ``A6-client-retries.md`` (see | ||
category definitions below) | ||
1. Write the RFC. | ||
1. Rename it to ``$CategoryName##-$Summary``, eg.: ``A6-client-retries.md`` | ||
(see category definitions below) | ||
- For language-specific proposals, include the name of the language: | ||
``L##-$Language-$Summary``. Canonical names: `core`, `cpp`, `csharp`, `go`, | ||
``L##-$Language-$Summary``. Canonical names: `core`, `cpp`, `csharp`, `go`, | ||
`java`, `node`, `objc`, `php`, `python`, `ruby`. | ||
1. Write up the RFC. | ||
1. Submit a Pull Request. | ||
1. Someone from gRPC team will be assigned as an APPROVER as part of this | ||
review. Once the APPROVER is assigned, the OWNER needs to start a discussion on | ||
[grpc-io](https://groups.google.com/forum/#!forum/grpc-io) and update the PR | ||
with the discussion link. After this is done, the OWNER should update the gRFC | ||
to the state of ``In Review``. It is expected that the APPROVER will help the | ||
OWNER along this process as needed. | ||
1. For at least a period of 10 business days (the minimum comment period), | ||
it is expected that the OWNER will respond to the comments and make updates | ||
to the RFC as new commits to the PR. Through the process, the discussion | ||
needs to be kept to the designated thread in the mailing list in order to | ||
avoid splintering conversations. The OWNER is encouraged to solicit as much | ||
feedback on the proposal as possible during this period. | ||
PR comments should be limited to formatting and vocabulary. | ||
- To determine the number to use for your proposal, view all PRs (open and | ||
closed), sorted by creation date ([link]( | ||
https://github.com/grpc/proposal/pulls?q=is%3Apr+sort%3Acreated-desc)). | ||
Find the first _new_ proposal PR of the same type, and use the following | ||
number. | ||
1. Submit a Pull Request. The PR description should be formatted as follows: | ||
|
||
$CategoryName##: <title> | ||
|
||
1. Someone from the gRPC team will be assigned as an APPROVER as part of this | ||
review. | ||
1. For a period of at least 10 business days (the minimum comment period), it is | ||
expected that the OWNER will respond to the comments and make updates to the RFC | ||
as new commits to the PR. The OWNER is encouraged to solicit as much feedback on | ||
the proposal as possible during this period. | ||
1. If there is consensus as deemed by the APPROVER during the comment period, | ||
the APPROVER will mark the proposal as final and assign it a gRFC number. | ||
Once this is assigned (as part of the closure of discussion), the OWNER will | ||
update the state of the PR as final and submit the PR. | ||
Commits must not be squashed; the commit history serves as a log of changes | ||
made to the proposal. | ||
the APPROVER will approve the PR on GitHub. The PR will then be merged by either | ||
the OWNER, if possible, or the APPROVER otherwise. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we maybe explicitly say something here like "A proposal being merged indicates that it has been approved", just to make the state clear? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems pretty clear from the preceding sentence to me... I added a following sentence to remove any doubt. PTAL. |
||
|
||
All proposals merged into the repo are considered "approved" and either | ||
implemented or ready to implement. | ||
|
||
## APPROVER | ||
|
||
- By default ``a11r`` is the approver unless another approver is assigned | ||
on a per-proposal basis. | ||
- If the assigned APPROVER and the OWNER cannot satisfactorily settle an issue, | ||
the final APPROVER is still ``a11r``. | ||
|
||
## Proposal Categories | ||
|
||
The proposals shall be numbered in increasing order. | ||
|
||
- ``#An`` - Affects all languages. | ||
- ``#Pnn`` - Affects processes, such as the proposal process itself. | ||
- ``#Lnnn`` - Language specific changes to external APIs or platform support. | ||
- ``#Gnnnn`` - Protocol level changes. | ||
|
||
## Proposal Status | ||
1. Every uncommitted proposal candidate starts off in the ``Draft`` state. | ||
1. After it accepted for review and posted to the group, it enters the | ||
``In Review`` state. | ||
1. Once it is approved for submission by the arbiter, it goes into the | ||
``Final`` state. Only minor changes are allowed (what qualifies as minor is | ||
left to the APPROVER). | ||
1. If a proposal needs to be revisited, it can be moved back to the ``Draft`` | ||
or ``In Review`` state. This can happen if issues are discovered during | ||
implementation. At which point, the review process as described above must be | ||
followed. | ||
1. Once a proposal is ``Final`` and if it has been implemented by a language, | ||
it can be updated to a status of ``Implemented`` with the implementing | ||
languages listed. (Listing versions is not required.) | ||
- ``An`` - Affects all languages. | ||
- ``Pnn`` - Affects processes, such as the proposal process itself. | ||
- ``Lnnn`` - Language specific changes to external APIs or platform support. | ||
- ``Gnnnn`` - Protocol level changes. | ||
|
||
## Updating proposals | ||
|
||
Sometimes small changes are needed to approved proposals. Rather than create | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do think we should have some hints for what "small" is.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly I think anything that's not considered a "major" change should be a modification of a gRFC instead of a new one. A major change would be like an entirely different API or a wholly different mechanism for solving the same problem. "Minor" as described below, IMO, is for editorial fixes and clarifying text, which is allowed to be approved without the 10 day waiting period. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the original
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, so that's where you got that language... I think then we should harken back to the original language.
This is potentially more restrictive, but it also doesn't change what updates can occur and how compared to the existing text. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry, but I don't like your wording. "Only minor updates typically occur" -- to say "only X is typical" is unusual and unnecessary. The goal of the original process here was:
I believe I expressed this adequately with my wording. I've made a small change here to move stuff / simplify. LMK if you disagree. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I disagree with your interpretation of the original text. I think the "This can happen if issues are discovered during implementation" part is important, as it provides a scope/context of the type of change we're talking about. We're not talking about redesigning old gRFCs 5 years after it was implemented because we no longer like that approach. Such things should be a new gRFC. Your proposed text encourages changing existing gRFCs, with no real limit on what sort of changes we'd expect to make. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a gRFC has been implemented we probably aren't going to be materially modifying it in place except possibly to correct a problem discovered later or add a necessary, small tweak. I take the "This can happen if" as an example, not to mean "This can only happen if". IMO we should leave it up to our judgement on a case-by-case basis whether it's better to update a gRFC or create a new one. That's realistically already what we are doing, and I don't see this minor wording change (done to remove now-unnecessary clauses with the removal of the "Status" field) affecting anything. @markdroth, thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree. But the example does provide a guide. If someone wanted to do a huge change we could say, "that's a bigger change than we'd expect for an approved gRFC" and they wouldn't be surprised, because you can see how far of a departure that huge change is from the example. That is gone in the new language, and the new language encourages making the change to the existing gRFC without any limit, implied or explicit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can see both sides of this. I agree with Doug that we should ultimately leave the decision to the approver. But I also agree with Eric that we don't want to leave this completely open-ended; I think we do want to make it clear that non-minor changes to an existing gRFC are discouraged. I will note that we do have one example where we made significant changes to a gRFC after it was already implemented, which was the modifications to A51 in #345. In hindsight, this really should have been done as a separate gRFC; I was the one who suggested modifying the existing gRFC, since I thought it would be a fairly simple change, but it turned out to be more complex than I expected. I think that having written guidance discouraging that might have prompted someone to object before that PR got merged. In general, my feeling is that any modification to an existing gRFC should be minor enough that it doesn't require the 10-day comment period. I think that in most cases, if we're considering needing the 10-day comment period, that's probably a signal that we should be writing a new gRFC rather than modifying an existing one. But I don't want to be prescriptive about this -- I do think we want to allow the approver to use their judgement. I suggest the following wording: """ To make changes to an already-merged proposal, a PR may be approved and merged at the OWNER's and APPROVER's convenience. Note that the APPROVER will decide whether this kind of post-merge change is appropriate. The approver will also determine whether the 10-day comment period is necessary, based on how significant the change is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems fine to me. Updated. |
||
new proposals for such changes, it is often better to revise the existing one. | ||
|
||
When updating a proposal, the PR description should be named as follows: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should say something about when and why we would update an existing gRFC instead of creating a new one? For example, maybe something like "Sometimes, after a gRFC is approved, small changes are needed to the design. In these cases, it is often simpler to modify the existing gRFC rather than create a new one." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done (with slightly different wording). |
||
|
||
$CategoryName## update: <description of change> | ||
|
||
If the update is minor (what qualifies as minor is left to the APPROVER), the PR | ||
may be approved and merged at the OWNER's and APPROVER's convenience. | ||
Otherwise, the original process's 10 day comment period must be observed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking that we should probably go through the existing gRFCs and remove the "Status:" field, since it's pretty useless and misleading. But we can do that in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I was thinking of the same, but maybe we should wait a month or two, since there are PRs pending with Status fields in them as well.