lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mike Drob <md...@apache.org>
Subject Re: Commit / Code Review Policy
Date Tue, 03 Dec 2019 19:41:59 GMT
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.
https://spark.apache.org/contributing.html
HBase requires a review and has an exception for trivial patches -
http://hbase.apache.org/book.html#_review
Yetus requires reviews, but allows binding review from non-committers, and
has a no review expiration. https://s.apache.org/mi0r8 - 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. -
https://zookeeper.apache.org/bylaws.html

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.

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.

Mike

On Mon, Dec 2, 2019 at 11:19 PM David Smiley <david.w.smiley@gmail.com>
wrote:

> https://cwiki.apache.org/confluence/display/LUCENE/Commit+Policy+-+DRAFT
>
> Updated:
> * Suggested new title
> * Emphasizing "Guidelines" instead of policy
> * Defines lazy-consensus
> * Added [PENDING DISCUSSION] to other topics for now
>
> Question:
> * Are we agreeable to my definition of "minor"?
> * Do we agree we don't even need a JIRA issue for "minor" things?
> * Do we agree we don't even need a CHANGES.txt entry for "minor" things?
> Of course it's ultimately up to the committer's discretion but I ask as a
> general guideline.  If we can imagine some counter examples then they might
> be good candidates to add to the doc.
>
> ~ David Smiley
> Apache Lucene/Solr Search Developer
> http://www.linkedin.com/in/davidwsmiley
>
>
> On Mon, Dec 2, 2019 at 10:15 PM Ishan Chattopadhyaya <
> ichattopadhyaya@gmail.com> wrote:
>
>> > Why should I ask for your review? It's not even your code thats
>> running anymore, its the hackers code :)
>>
>> Haha! +1 on moving ahead with RCEs and other security issues without
>> needing to wait for reviews. Waiting for reviews (esp. if no one has enough
>> bandwidth for quick reviews) for such crucial issues can risk dragging
>> those issues on and on needlessly. Reviews can happen after commit too, if
>> people have the time.
>>
>> On Tue, 3 Dec, 2019, 6:51 AM Robert Muir, <rcmuir@gmail.com> wrote:
>>
>>>
>>>
>>> On Mon, Dec 2, 2019 at 3:33 PM David Smiley <david.w.smiley@gmail.com>
>>> wrote:
>>>
>>>>
>>>> Rob wrote:
>>>>
>>>>> Why should I wait weeks/months for some explicit review
>>>>>
>>>> Ask for a review, which as this document says is really just a LGTM
>>>> threshold of approval, not even a real code review.  Given your reputation
>>>> of writing quality code, this isn't going to be an issue for you.  If it's
>>>> taking multiple weeks for anyone then we have a problem to fix -- and at
>>>> present we do in Solr.  Explicitly encouraging mere approvals (as the
>>>> document says) should help a little.  Establishing that we want this
>>>> standard of conduct as this document says (even if not mandatory) will also
>>>> help -- "you scratch my back, I scratch yours".  But I think we should do
>>>> even more...
>>>>
>>>>
>>>  Why should I ask for your review? It's not even your code thats running
>>> anymore, its the hackers code :)
>>>
>>>
>>>

Mime
View raw message