Hey Druids, Sometimes I feel like this too: https://twitter.com/julianhyde/status/1108502471830204416.
I believe our code review process today has too much friction in it, and that we can work to reduce it. The two main frictions I see are code reviews not happening in a timely manner, and code reviews sometimes descending into a swamp of nit-picks. The good news, at least, is that we are not the first development team to have these problems, and that they can be solved. I've written some thoughts below about principles that I think might help. I am not necessarily proposing making these the law of the land, but am hoping to start a discussion about how we can generally improve things. 1) "Let robots handle style checks." Encode Druid's code style in checkstyle or other tools, and avoid making subjective style comments directly as humans. If we can't figure out how to set up automated verification for a particular style point, then give it up. Rationale: authors can self-check style when checking is automated. Also, it's better for robots to enforce style, because software development is a social endeavor, and people don't mind style nit-picking from robots as much as they do from humans. 2) "Write down what really matters." I suggest we put together a short, highly visible list of things that should be considered commit-blockers for patches. Not a list of best practices, but a list of true blockers. "Short" is in service of point (3), below. "Highly visible" is so it can act as a shared set of values. My suggested list would be correctness of implementation, documentation of new or changed functionality, tests for reasonably testable functionality, avoidance of excessive additional maintenance burden, and avoidance of breaking existing use cases (including things that would break clusters that run at large scale, or that would break rolling updates). Some of these points are subjective but I think it's still a good start. Rationale: authors will know what is expected of them, which should generally improve PR quality, and speed up review. Also, similar to the previous point: people are generally happy to follow what they perceive as community-maintained standards, but not as happy to follow what they perceive as the opinion of a single reviewer. 3) "Minimize hoop-jumping." We should make an effort to avoid the '74 one-line emails' problem. Endeavor to make fewer and fewer comments as the number of rounds of review of a PR gets higher. Endeavor to keep the number of rounds of review from getting too high. Authors can help here by explaining their decisions clearly, and by avoiding making large changes in their patches after review has started. Reviewers can help by making their first review as comprehensive as possible, to avoid the need to bring up brand-new points on later rounds of review. Reviewers can also help by writing out changes they're asking for explicitly (suggest a new comment, doc wording, method name, etc). Reviewers can also help by noting in a review comment when a suggestion is just a suggestion -- useful because many authors are likely to interpret an unqualified comment as a commit blocker. Rationale: the more rounds of review a PR goes through, and the more a review feels like a collection of disconnected 1-liner comments, the more it makes the original author feel as if he or she is being made to jump through hoops. This makes people less likely to contribute in the future, and damages efforts to build community. 4) "Pitch in on reviews." A relatively small number of committers are doing most of the code reviews. These people have limited time, and it means that PRs often stay in the review queue for a while without anyone looking at them. Any committer can pitch in, and in fact, even non-committers can (although their votes are nonbinding, their reviews are still helpful for social reasons). So anyone interested in reviewing is encouraged to do so. Rationale: improves bandwidth, prevents burning out the small number of volunteers that participate in reviews. Looking forward to seeing what people in the community think. Gian
