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

Update readme and template to reflect actual process being followed #275

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 15 additions & 9 deletions GRFC-TEMPLATE.md
Expand Up @@ -2,21 +2,19 @@ Title
----
* Author(s): [Author Name, Co-Author Name ...]
* Approver: a11r
* Status: {Draft, In Review, Ready for Implementation, Implemented}
Copy link
Member

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.

Copy link
Member Author

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.

* Implemented in: <language, ...>
* Last updated: [Date]
* Discussion at: <google group thread> (filled after thread exists)
ejona86 marked this conversation as resolved.
Show resolved Hide resolved

## Abstract

[A short summary of the proposal.]

## Background

[An introduction of the necessary background and the problem being solved by the proposed change.]
[An introduction of the necessary background and the problem being solved by the
proposed change.]


### Related Proposals:
### Related Proposals:
* A list of proposals this proposal builds on or supersedes.

## Proposal
Expand All @@ -25,17 +23,25 @@ Title

### Temporary environment variable protection

[Name the environment variable(s) used to enable/disable the feature(s) this proposal introduces and their default(s). Generally, features that are enabled by I/O should include this type of control until they have passed some testing criteria, which should also be detailed here. This section may be omitted if there are none.]
[Name the environment variable(s) used to enable/disable the feature(s) this
proposal introduces and their default(s). Generally, features that are enabled
by I/O should include this type of control until they have passed some testing
criteria, which should also be detailed here. This section may be omitted if
there are none.]

## Rationale

[A discussion of alternate approaches and the trade offs, advantages, and disadvantages of the specified approach.]
[A discussion of alternate approaches and the trade offs, advantages, and
disadvantages of the specified approach.]


## Implementation

[A description of the steps in the implementation, who will do them, and when. If a particular language is going to get the implementation first, this section should list the proposed order.]
[A description of the steps in the implementation, who will do them, and when.
If a particular language is going to get the implementation first, this section
should list the proposed order.]

## Open issues (if applicable)

[A discussion of issues relating to this proposal for which the author does not know the solution. This section may be omitted if there are none.]
[A discussion of issues relating to this proposal for which the author does not
know the solution. This section may be omitted if there are none.]
117 changes: 64 additions & 53 deletions README.md
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

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

I do think we should have some hints for what "small" is.

Sometimes small changes are needed to approved proposals. For example, editorial fixes, clarifying text, and detailed fixes noticed during implementation or usage. Rather than create new proposals for such changes, it is often better to revise the existing one.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the original README.md text, for reference:

  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).
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Only minor updates typically occur after a proposal is approved (and merged?). Minor updates do not require a comment period. What qualifies as minor is left to the APPROVER. If an update is needed and is not minor, then the process's 10 day comment period must be observed. This can happen if issues are discovered during implementation.

When updating a proposal, the PR description should be named as follows:
$CategoryName## update:

This is potentially more restrictive, but it also doesn't change what updates can occur and how compared to the existing text.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only minor updates typically occur after a proposal is approved

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:

  1. "Minor" changes to a gRFC are allowed with no 10-day discussion period.
  2. Bigger changes to a gRFC are allowed with the 10-day discussion period.

I believe I expressed this adequately with my wording. I've made a small change here to move stuff / simplify. LMK if you disagree.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

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

I take the "This can happen if" as an example, not to mean "This can only happen if".

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.

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:
Copy link
Member

Choose a reason for hiding this comment

The 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."

Copy link
Member Author

Choose a reason for hiding this comment

The 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.