Re: Code review

[Roman Leventov <le...@gmail.com>]

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 <fa...@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 <glasser@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 <gi...@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-unsubscribe@druid.apache.org
> > For additional commands, e-mail: dev-help@druid.apache.org
> >
> >
>

Re: Code review

[Roman Leventov <le...@gmail.com>]

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 <le...@gmail.com> 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 <fa...@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 <
>> glasser@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 <gi...@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-unsubscribe@druid.apache.org
>> > For additional commands, e-mail: dev-help@druid.apache.org
>> >
>> >
>>
>