rfcs: update the RFC template #60144
Conversation
|
This change is |
knz
force-pushed
the
20210209-rfc-update
branch
2 times, most recently
from
b96ac43 to
e4ed12a
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, and @knz)
docs/RFCS/00000000_template.md, line 40 at r1 (raw file):
Some of these prompts may not be relevant to your RFC; in which case you can spell out “this change does not affect ...” or answer “N/A” (not applicable) next to the question.
I like that you have prompts for observability and performance below. I think it would be useful to provide prompts regarding stability. For example, "Can this new functionality affect the stability of a node or the entire cluster? Can the new functionality be disabled? Can the new functionality affect clusters which are not explicitly using it? What testing and safe guards are being put in place to protect against unexpected problems?"
docs/RFCS/00000000_template.md, line 61 at r1 (raw file):
high-level worst case algorithmic complexity? How is resource usage affected for “large” loads? For example, what do we expect to happen when there are 100.000 ranges? 10.000 users? 1000.000 concurrent
Nit: use commas for large numbers, not periods (yes, this is US centric, but I'm pretty sure that is what we do elsewhere).
There was a problem hiding this comment.
Reviewable status:
docs/RFCS/00000000_template.md, line 40 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
I like that you have prompts for observability and performance below. I think it would be useful to provide prompts regarding stability. For example, "Can this new functionality affect the stability of a node or the entire cluster? Can the new functionality be disabled? Can the new functionality affect clusters which are not explicitly using it? What testing and safe guards are being put in place to protect against unexpected problems?"
Done.
docs/RFCS/00000000_template.md, line 61 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Nit: use commas for large numbers, not periods (yes, this is US centric, but I'm pretty sure that is what we do elsewhere).
Done.
knz
force-pushed
the
20210209-rfc-update
branch
2 times, most recently
from
bae04be to
2bfd8b9
Compare
|
No specific comments but just want to say that this format of having 1 clear bullet point/question per topic makes the RFC process much more approachable to me ✨ thanks for reworking this @knz and @andreimatei |
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
Rafa, Andrei, thanks for spending the time you did here. I think it's a marked
improvement over what we had previously. I have a mild opinion I'll share here
that I think will further encourage usage of github-based at CRL.
I'm coming at this from the perspective as someone who really likes starting
with an empty page and just writing things out free-form first, to think.
It's only afterwards that I come back to make sure that I've dotted the i's and
crossed the t's.
The "template" then that I want to start off with is preferably empty, with
just the fewest guardrails to inform my overall structure (and no more). It's
only afterwards that I (and my reviewers!) should want to refer to a
check list of things to make sure we've covered all the relevant bases.
For that reason I think that most of the prompts here are better placed in the
README instead, or a separate (linked) "design considerations" doc.
I don't think this is minor, it's a quality of life improvement. I partially
attribute the blank-slated-ness of gdocs to it's increased usage over our
existing RFC process. I'll contrast this to the starting point being a long
checklist, where it's easier for me then to just start elsewhere (and taking
the discussions there with me).
The "UX" of the template itself informs how the design process takes place, so
the fewer questions/bullet points to answer at the outset, the better.
Entrusting the RFC author (and the reviewers) to walk through the prompts
placed elsewhere seems fine to me.
Instead of "reference-level explanation", how about "technical design"?
Instead of "guide-level explanation", how about "explain it to someone else"?
The smallest set of guardrails, in my view, are: summary, motivation or
background, technical design (incl. drawbacks and alternatives), unresolved
questions, and explaining it to someone else. I'd vastly prefer a template with
at most a paragraph per subheading explaining roughly what should go in there,
and linking to a separate doc for more details.
There was a problem hiding this comment.
Instead of "reference-level explanation", how about "technical design"? Instead of "guide-level explanation", how about "explain it to someone else"?
Done.
I'd vastly prefer a template with at most a paragraph per subheading explaining roughly what should go in there, and linking to a separate doc for more details.
Ok done, PTAL.
Reviewable status:
knz
force-pushed
the
20210209-rfc-update
branch
from
2bfd8b9 to
8096c5c
Compare
There was a problem hiding this comment.
Reviewable status:
docs/RFCS/00000000_template.md, line 40 at r2 (raw file):
... # Explain it to someone else
when I saw this section in your recent RFC, it struck me. The way in which you've chosen to fill it I think suggests its usefulness :)
I think we should just get rid of it.
There was a problem hiding this comment.
Reviewable status:
docs/RFCS/GUIDE.md, line 8 at r2 (raw file):
Suggested contents: - What is being proposed - Why (short reason)
Nit: To match the order below, we should move "Why" up to the top in this list.
docs/RFCS/GUIDE.md, line 35 at r2 (raw file):
- Questions about the change: - What components in CockroachDB need to change? How do they change?
Nit: These prompts are great, and could benefit from additional cues to ensure that people are thinking of all aspects of the system. For example, are there security implications to this change? Will this require changes to work properly in CC, or multi-tenant serverless environments? We discuss migration in the "teaching" section below, but we might also want to prompt for it up here, as there could be technical challenges there too.
docs/RFCS/GUIDE.md, line 62 at r2 (raw file):
- Questions about performance: - Does the change impact performance? How?
Nit: We might also want to add a question here about testing of performance, if there's an anticipated impact. We call out testing below, but people might not clue into the need for performance testing.
knz
force-pushed
the
20210209-rfc-update
branch
2 times, most recently
from
fd967bd to
ec5736d
Compare
There was a problem hiding this comment.
Reviewable status:
docs/RFCS/00000000_template.md, line 40 at r2 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
when I saw this section in your recent RFC, it struck me. The way in which you've chosen to fill it I think suggests its usefulness :)
I think we should just get rid of it.
That other RFC is actually not complete. I made a mistake showing it to you in that state. The section is still as useful as ever, even if you personally don't like it.
docs/RFCS/GUIDE.md, line 8 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Nit: To match the order below, we should move "Why" up to the top in this list.
I disagree, I think every RFC should start with its punchline. At least as long as the majority of the readers are from the US. US folk are not able to process a text where the first sentence is not also the conclusion.
docs/RFCS/GUIDE.md, line 35 at r2 (raw file):
For example, are there security implications to this change? Will this require changes to work properly in CC, or multi-tenant serverless environments?
Good points. Added.
We discuss migration in the "teaching" section below, but we might also want to prompt for it up here, as there could be technical challenges there too.
I don't understand this. Can you tell more?
There was a problem hiding this comment.
Reviewable status:
docs/RFCS/GUIDE.md, line 35 at r2 (raw file):
Previously, knz (kena) wrote…
For example, are there security implications to this change? Will this require changes to work properly in CC, or multi-tenant serverless environments?
Good points. Added.
We discuss migration in the "teaching" section below, but we might also want to prompt for it up here, as there could be technical challenges there too.
I don't understand this. Can you tell more?
Maybe it's nothing, but I'd think that running in mixed version clusters could present issues for at least some RFCs. People would have to consider the need to either block a feature from running in that mode, or ensure that it runs there correctly.
Throughout the last year usage of the RFC system decreased, and surveys revealed that folk find the template too difficult to use. After analyzing options, @andreimatei and I came to the following: - The existence of “guide-level explanation” and “reference-level explanation” side by side is confusing. It is unclear what needs to be present in one and not the other. - Engineers dislike looking at “guide-level explanation” first, since the RFCs are (and should be) mostly used to stimulate technical discussions, and so the technical aspects should come first. - The writing prompts in each section are too vague and do not actually do a good job of guiding the writer. To address this shortcoming, this PR does the following: - Move the "Guide-level explanation" to the bottom. - Change the writing guides to clear questions, one bullet point and one question per topic. Release note: None
There was a problem hiding this comment.
Reviewable status:
docs/RFCS/00000000_template.md, line 40 at r2 (raw file):
Previously, knz (kena) wrote…
That other RFC is actually not complete. I made a mistake showing it to you in that state. The section is still as useful as ever, even if you personally don't like it.
I added the missing text to that section in the other RFC :)
docs/RFCS/GUIDE.md, line 35 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Maybe it's nothing, but I'd think that running in mixed version clusters could present issues for at least some RFCs. People would have to consider the need to either block a feature from running in that mode, or ensure that it runs there correctly.
Oh good point! Added a prompt for this.
docs/RFCS/GUIDE.md, line 62 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Nit: We might also want to add a question here about testing of performance, if there's an anticipated impact. We call out testing below, but people might not clue into the need for performance testing.
I don't know that this is the role of any RFC to call out performance testing -- in the same way that we do not require other RFCs to spell out the architecture of unit tests.
We do need documentation in wiki.crdb.io about how to design for tests and how to design tests. We do not have such documentation today and I agree it is sorely missing. In that documentation, we should call out performance testing and guide readers towards learning how to do it effectively.
knz
force-pushed
the
20210209-rfc-update
branch
from
ec5736d to
b59bc2a
Compare
|
I'm going to merge this now. As usual if we need/want more adjustments we can do so in a followup PR. bors r=bdarnell,petermattis,ajstorm,andreimatei,irfansharif |
|
Build failed: |
|
Unrelated flake. bors r=bdarnell,petermattis,ajstorm,andreimatei,irfansharif |
|
Build failed: |
|
gosh, what a nuisance bors r=bdarnell,petermattis,ajstorm,andreimatei,irfansharif |
|
Build succeeded: |
Throughout the last year usage of the RFC system decreased, and
surveys revealed that folk find the template too difficult to use.
After analyzing options, @andreimatei and I came to the following:
The existence of “guide-level explanation” and “reference-level
explanation” side by side is confusing. It is unclear what needs to be
present in one and not the other.
Engineers dislike looking at “guide-level explanation” first, since
the RFCs are (and should be) mostly used to stimulate technical
discussions, and so the technical aspects should come first.
The writing prompts in each section are too vague and do not
actually do a good job of guiding the writer.
To address this shortcoming, this PR does the following:
Move the "Guide-level explanation" to the bottom.
Change the writing guides to clear questions, one bullet point and
one question per topic.
Release note: None