I've created the following PR based on this discussion and the document that I've started to prepare last year: https://docs.google.com/document/d/17EEKT6fih9Dd5NfXjBoECcKbVp1eOB2vb3jKqTF9pPc/edit .
https://github.com/apache/incubator-druid/pull/7991 I believe having agreed upon development principles will be a strong foundation for future community collaboration. Please review. On Mon, 13 May 2019 at 18:29, Roman Leventov <[email protected]> wrote: > 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 <[email protected]> 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 <[email protected]> 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 < >> [email protected]> >> > 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 <[email protected]> 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: [email protected] >> > For additional commands, e-mail: [email protected] >> > >> > >> >
