ADR for reviewer and commiter responsibilities #44097
Conversation
emmanuelbernard
force-pushed
the
adr-review
branch
from
651be3e
to
91803c2
Compare
| * Being a perfect solution is _not_ a goal, _better_ than today is. | ||
| * Define ways to measure and improve the time to review. | ||
|
|
||
| == Scenarios (optional) |
|
|
||
| == Considered options | ||
|
|
||
| The main alternative is the status quo which roughly is: Guillaume knowing what we are intending to do and some persons talking with Guillaume regularly being sufficiently aware. |
There was a problem hiding this comment.
That's not true. While Guillaume provides extensive review on many areas, he is not alone to overlook the whole thing.
There was a problem hiding this comment.
This sentence is not talking about the review, it is talking about the process for the reviews.
|
|
||
| There are three levels of reviewing. | ||
|
|
||
| . Any one can review a Pull Request (PR) and give constructive feedback on different areas (code, documentation, feature experience, security, etc.). |
There was a problem hiding this comment.
Is it intended to be a * ? or a 1.
You used * above.
There was a problem hiding this comment.
yes because it's about 3 levels, I felt numbering was helpful. I can change if that hurts too many people.
| More eyes means higher quality. | ||
| . A PR is reviewed by a _binding reviewer_ who is fluent in the area being touched: this person is responsible for signing with blood of the usefulness, quality of the code and soundness of the solution. | ||
| That’s the review of the expert and the most critical one for quality. | ||
| This review is mandatory. |
There was a problem hiding this comment.
PRs are associated with areas (when not, they get the need-triage label, and it's quickly identified, and the areas are set manually). Each area has a set (sometimes a set of 1) of binding reviewers. They are listed in https://github.com/quarkusio/quarkus/blob/main/.github/quarkus-github-bot.yml.
| . A PR is reviewed by a _binding reviewer_ who is fluent in the area being touched: this person is responsible for signing with blood of the usefulness, quality of the code and soundness of the solution. | ||
| That’s the review of the expert and the most critical one for quality. | ||
| This review is mandatory. | ||
| .. Unfortunately, we do not always have (designed or effective) experts in all areas so it’s a constant work in progress and might make this step optional in some situations. |
There was a problem hiding this comment.
It is rarely the case - we try to avoid that as much as possible. When this happens, we require more extensive reviews from multiple reviewers (Guillaume, Georgios, George, Alexey, myself, Max...)
There was a problem hiding this comment.
Might be worth talking with the team, there seems to be a perception reality disconnect from some.
| And how to use it as a mechanism to improve and know where to get help? | ||
| The faster the flow, the high an organisation measure in effectiveness, happiness etc. | ||
|
|
||
| ==== Proposition 1 |
There was a problem hiding this comment.
IF there is only one, let's drop the 1.
There was a problem hiding this comment.
It's really the meat to discuss, we could get more proposals from others. After the conversation, I anticipated "TODO Open questions to resolve before closing" will be folded into the other sections.
|
|
||
| ==== Proposition 1 | ||
|
|
||
| Define and measure (GitHub APIs) SLO for the time to push or drop / withdraw. |
There was a problem hiding this comment.
How would you define that magic number? As explained above, raising awareness is okay (and we have the lottery for this), but taking action is NOT.
And maybe this issue does not really exist? Do you have examples of PRs that took months to merge? It is not rare for a few weeks to be required because Quarkus has a complex (and pretty large) code base. Some PR have been discussed for multiple months, but that's a different issue.
There was a problem hiding this comment.
Yes look above, your comment made me do some GitHub queries to try and get some numbers around staleness.
| ==== Proposition 1 | ||
|
|
||
| Define and measure (GitHub APIs) SLO for the time to push or drop / withdraw. | ||
| This is not a perfect proxy as some PRs do demand multi weeks conversations but measuring and dashboarding is our first step to know in which area we have a problem. |
| Define and measure (GitHub APIs) SLO for the time to push or drop / withdraw. | ||
| This is not a perfect proxy as some PRs do demand multi weeks conversations but measuring and dashboarding is our first step to know in which area we have a problem. | ||
|
|
||
| We could define a team intended SLO (say 3 weeks to start but that’s already quite long) and know which areas chronically fall behind. |
There was a problem hiding this comment.
Three is not that long.
There is also another issue. Let's imagine 2 PRs are opened at the same time. One gets expedited quickly and they other one is staled. There are multiple reasons for this:
- we often trust more regular contributors - that isn't very objective, but that's the reality
Some PRs are more complex to review than others. Changing the native compilation configuration can be pretty touchy and requires the reviewer to test many possibilities. - some PR requires expertise even binding reviewers do not have (like Windows :-)) - cross-cutting PRs can also be tricky (because it requires multiple binding reviews, and technical friction may happen)
There was a problem hiding this comment.
Yes, that's where it's important to have the conversation.
- what are the useful SLO (is that 90% percentile vs absolute number)
- is a latency between last user comment and committer and binding reviewer comment useful (e.g. 95% before 7 days)
And even before defining the SLO, probably defining and measuring the SLI could be useful as an indicator.
|
|
||
| We could define a team intended SLO (say 3 weeks to start but that’s already quite long) and know which areas chronically fall behind. | ||
|
|
||
| We can introduce the notion of binding reviewer leutenants (like in the Linux kernel) where a binding reviewer whose area is not meeting the SLO would train other peoples (actively delegating the PR work as a first phase before its own review, mentoring, answering questions, etc). |
There was a problem hiding this comment.
| We can introduce the notion of binding reviewer leutenants (like in the Linux kernel) where a binding reviewer whose area is not meeting the SLO would train other peoples (actively delegating the PR work as a first phase before its own review, mentoring, answering questions, etc). | |
| We can introduce the notion of binding reviewer lieutenants (like in the Linux kernel), where a binding reviewer whose area is not meeting the SLO would train other people (actively delegating the PR work as a first phase before its own review, mentoring, answering questions, etc.). |
There was a problem hiding this comment.
I like the idea, but I'm not sure of the implementation. For me, even lieutenants would require a set of contributions before being named lieutenants and if they do that, they could become binding reviewers. Otherwise, it would mean that the last review (the merge part) would need to do a more extensive review (which would add even more latency)
There was a problem hiding this comment.
That's good input. What's the most important to me is to surface these rules that are in your head or in guillaume's head (and certainly not in my head), and write them down.
So maybe it's more a binding reviewer in training for an area (mentee concept) to help towards the contrubition levels you mentioned earlier.
|
I think we need to start with an ADR explaining the current rules and some magic GitHub invocation to track staled PRs. It does not mean that rules cannot be revisited, but having them written down would help. |
We had a discussion on reviewer responsibilities on Zulip.
In that thread, I captured the responsibilities as a summary.
I expanded that work and formalized it in an ADR.
I also expanded the conversation on the impact of time to PR merge and how we could improve this.
This is a draft as we want to further discuss this collectively.