A quote from Steve Tockey regarding documentation and comments and the cost of maintaining software: https://medium.com/@leventov/steve-tockey-on-software-documentation-and-comments-470751227a63
On Mon, 29 Apr 2019 at 19:30, Fangjin Yang <fang...@imply.io> wrote: > 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 > > > > >
