Strong +1 agreement with Gian. I definitely see there are anal code reviews focused too much on trying to force changes that make no difference to the core functionality being contributed, and are instead a reflection of personal biases of the reviewer. These types of low quality code reviews provide absolutely no value to the Druid community, and instead serve to damage the community engagement, and slow down development.
On Thu, Apr 25, 2019 at 11:48 AM Julian Hyde <jh...@apache.org> wrote: > +1 > > As someone who reviews Druid release candidates but does not develop Druid > code, I wish that “mvn test” would “just work”. I can imagine that > first-time and occasional contributors are in a similar boat to me. > > I know this is not an easy thing to achieve, because the test suite also > has to work for more experienced contributors. > > Julian > > > > On Apr 25, 2019, at 11:42 AM, David Glasser <glas...@apollographql.com> > wrote: > > > > As a new contributor, I've actually really appreciated the careful and > high > > quality code reviews I've received (primarily from Jihoon). > > > > The real source of friction for me has not been the feedback from > > developers, but the painfulness of running the tests. > > > > - Figuring out how to locally run the full suite of tests that CI will > run > > is not super documented. > > - Understanding how to just run the tests that are most relevant to your > > change (either from CLI or IntelliJ) isn't well documented. It's > > especially unclear what things can be perfectly successfully run from > > IntelliJ's own test runners and what things really require you to run > them > > through Maven. (Druid is also the first time I've used Maven directly > and > > it's a huge challenge for me to understand what's going on and how to run > > it properly.) > > - Many of the tests are just very slow and maybe could be parallelized > > better? > > > > It's enough of a pain that I often when iterating wouldn't even bother > > trying to run tests manually but would just push to GitHub and let Travis > > run it instead. But Travis itself is very slow and while it's somewhat > > parallelized, it's not super parallelized — and as a lowly PR author I > > can't even kill the overall job if I push a new version of the PR or if a > > sub-job already failed. So this would frequently add "round trip" times > of > > half an hour or more in the middle of trying to take the good advice from > > reviewers to heart. > > > > So while I agree that it would be great to move as many of the checks as > > possible into CI from "core dev teams mind", I'd also encourage folks > with > > more time and expertise than I have now to put effort into making the CI > > experience faster and easier. (I wonder if some of the issues could just > > be solved with money, eg running on more powerful Travis machines or > > parallelizing the slower tests even further onto a larger number of > > containers. I've also generally found in my own work that CircleCI at > > least feels faster and easier to work with than Travis FWIW.) > > > > --dave > > > > On Mon, Apr 22, 2019 at 7:44 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. > >> > >> 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 > >> > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org > For additional commands, e-mail: dev-h...@druid.apache.org > >
