On Mon, Apr 22, 2019 at 7:43 PM Gian Merlino <g...@apache.org> wrote:
> 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. > OMG Gian, are you taking away all the fun? what is code review without arguing about the meaning of life and code style, variable names etc ?? Totally agree, we need to avoid as much as possible commenting on code style if it is not possible to write an automated rule to enforce it. Let the computer do the fun/boring stuff ! > 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. > Kind of hard to define those but am okay with putting up a draft of what can be really a commit blocker. FYI the apache way allow committers to use -1 as a blocker so maybe let's keep it that way, not sure tho. > > 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. > This should go both way too, authors need to understand that making the pull request as light as possible is the key to make review much simpler. Sometimes i see PR that refactor 1000s of line to fix a one line bug, that is not cool, i personally can not read more than 400 lines in one go, when reviewing. It will be nice if the authors can break their PR into set of change-lists of about 300 lines each that will help lazy ppls like me to take a look. Another point is to keep it clear what the PR is aiming at, and how a reviewer might look at it and what to ignore... > > 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 >
