lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Smiley <>
Subject Re: Commit / Code Review Policy
Date Wed, 04 Dec 2019 20:34:02 GMT
Mike D.,
  I loved your response, especially for researching what other projects do!

... more responses within...

On Tue, Dec 3, 2019 at 2:42 PM Mike Drob <> wrote:

> I'm coming late into this thread because a lot of the discussion happened
> while I was offline, so some of the focus has shifted, but I'll try to
> address a few topics that were brought up earlier anyway. In an effort to
> be brief, I'll try to summarize sentiments rather than addressing specific
> quotes - if I mischaracterize something please let me know!
> First, I don't believe that RTC requires consensus voting with 3 reviews
> per commit or anything nearly so burdensome. A brief survey of other Apache
> projects shows that most on RTC only need a single review, and also can
> include exceptions for trivial changes like we are discussing here. If
> we're trying to find a compromise position, I personally would prefer to be
> more on the RTC side because I think it's a more responsible place to be
> for a project that backs billions of dollars of revenue across hundreds of
> companies annually.
> Spark is pretty strict RTC, but with such a volume of contributions it may
> be the only option sustainable for them.
> HBase requires a review and has an exception for trivial patches -
> Yetus requires reviews, but allows binding review from non-committers, and
> has a no review expiration. - there's a lot of
> other good discussion there too.
> Zookeeper requires a minimum one day waiting period on all code change,
> but does not require explicit reviews. -
> One piece I'll echo from the Yetus discussion is that if we have a habit
> of review, then we're less likely to get stagnant patches, and we're also
> more likely to engage with non-committer contributions. If we don't have
> that habit of reviews, then patches do get stale, and
> trust/self-enforcement becomes the only sustainable way forward.
> A second point that I'm also concerned about is the idea that we don't
> want JIRA issues or CHANGES entries for trivial or even minor patches. This
> has a huge impact on potential contributors and their accessibility to the
> project. If contributors aren't credited for all of their work, then it
> makes it harder for them to reach invitation as a committer. As a personal
> example, a lot of my changes were around logging and error handling, which
> are minor on your list but fairly important for a supporter in aggregate.
> If the community signalled that the work was less valuable, less visible,
> and less likely to be accepted (each of which can follow from the previous
> I think) then I don't know that I would have been motivated to try and
> contribute those issues.

Our CHANGES.txt tries to simultaneously be a useful document in informing
users (and us) of what was changed for what issue that we might actually
care about, and *also* give kudos to contributors.  There is a lot of noise
in there, as it is.  If hypothetically a contributor files a JIRA issue
with minor/trivial priority, then maybe a git author tag is enough?  Or if
not then maybe adding a special section in the CHANGES.txt for a special
thanks to contributors of unspecified issues?

> To the point about security issues, that's something that should probably
> be addressed explicitly on the security/private list, and it absolutely
> needs review if only so that other developers can learn and avoid those
> issues again. That's where the power of community is really important, and
> I don't expect issues like that to sit around with a patch waiting for very
> long anyway.
> Overall, I think following in the Yetus or ZK model with a 72 hour timeout
> is a reasonable compromise, especially because a hard shift from CTR to RTC
> would need a corresponding culture shift that may not happen immediately.

Yes I agree.  Can you suggest a proposed guideline (or perhaps actual
policy?  hmm)?  Today our guidelines don't quite have this rule, which thus
allows a committer to commit a major change immediately without a review
(ouch!).  Surely we don't *actually* want to allow that.

> Mike

View raw message