lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jan Høydahl <jan....@cominvent.com>
Subject Re: Commit / Code Review Policy
Date Mon, 02 Dec 2019 08:49:22 GMT
I think the distanse is not necessarily as large as it seems. Nobody wants to get rid of lazy
consensus, but rather put down in writing when we recommend to wait for explicit review.

There have been examples of some rather sloppy and rapid commits in Solr of non-trivial core
code that turned out to cause bugs, corruption and perhaps even security issues. 

Sloppy commit culture (in Solr land) was also one thing Mark pointed out as a major threat
to Solr’s stability and I share this concern. That is what originally triggered the committees
meeting and David’s effort with this document.

I applaude David’s will to try improve the situation and I hope we all can contribute and
be part of the solution. We’re a great team!

Robert, I fully support your RCE suggestions, I.e “looks good to me” which means I don’t
have the expertise to do any better :) 

Jan Høydahl

> 2. des. 2019 kl. 09:00 skrev Robert Muir <rcmuir@gmail.com>:
> 
> 
> yes David: I read the document, I'm against both the contents of it, and how it came
about.  For example there is no "silence is consensus" which is really important to me. If
you work full-time on this shit for some large company X, this is probably of no concern to
you.
> 
> But it matters to me. For example, I opened some patches to improve solr's security because
its currently an RCE-fest. I'm gonna wait a couple days, if nobody says anything about these
patches I opened for Solr, I'm gonna fucking commit them: someone needs to address this stuff.
Why should I wait weeks/months for some explicit review? There is repeated RCE happening,
how the hell could I make anything worse?
> 
> Please don't add unnecessary things to this document like "Linear git history" which
have nothing to do with code reviews. Its already controversial as its trying to invent its
own form of "RTC", you aren't helping your cause.
> 
> 
>> On Mon, Dec 2, 2019 at 12:35 AM David Smiley <david.w.smiley@gmail.com> wrote:
>> Hello Rob, 
>> 
>> On Sat, Nov 30, 2019 at 9:39 AM Robert Muir <rcmuir@gmail.com> wrote:
>>>> On Tue, Nov 26, 2019 at 12:34 AM David Smiley <david.w.smiley@gmail.com>
wrote:
>>>>> Last Wednesday at a Solr committers meeting, there was general agreement...
>>>>> 
>>>>> I'd prefer we have one "Commit Policy" document for Lucene/Solr and only
call out Solr specifics when applicable.  This is easier to maintain and is in line with the
joint-ness of Lucene TLP.  So I think it should move to the Lucene cwiki.  Granted there is
a possibility this kind of content might move into our source control somewhere but that possibility
is a subject for another day.
>>>> 
>>>> -1 ... you even went so far as to discourage lucene committers from attending
that meeting, and now its turned around as if its consensus everywhere and should be applied
to lucene too?
>>> 
>>> I'm looking back over what I wrote.  I suppose it's this self-quote that bothered
you?:
>>> This particular committer's meeting has a particular subject/theme.  So as Erick
indicated, while all committers are invited, I think if you're not interested in the subject
then I'm sure you can find a better use of your time.
>> Personally I think what I stated there is reasonable.  More importantly, both me
and Erick also expressly declared all committers are welcome.  If I actually meant otherwise
then I wouldn't have said so.
>>  
>>> I don't think changing things to review-then-commit will help.
>> 
>> I don't think anyone wants ASF's RTC due to the "consensus approval" requirement
with three +1's -- http://www.apache.org/foundation/glossary.html#ReviewThenCommit  My proposal
says this up front clearly (I think).  Did you read it?  I don't think Shawn read it either
-- I'm not calling for an official change to RTC.  Here's the link again: https://cwiki.apache.org/confluence/display/LUCENE/Commit+Policy+-+DRAFT
>> I can copy-paste it here if you like which would also serve to snapshot exactly what
I'm talking about.
>> 
>> I observe that we seem to mostly follow this guideline document already, particularly
what I observe in Lucene (less so Solr).  No not all the details and all the particulars I
wrote on what constitutes "minor", but the gist of the review/approval.  We don't just commit
at will today; we wait for reviews and get them.  I'd like what we do today (in Lucene) written
down more clearly for everyone's benefit -- new/interested people and ourselves.  This document
is my attempt to do that plus raising the review/approval guideline norms just a little bit
(IMO).  For Solr it's more than a little bit, granted.  Then I can point others at this, and
if I see behavior that got no review for something where it was warranted (according to documented
guidelines), I can reference this.
>> 
>> Lets say we accept these new guidelines.  After six months, we can change/loosen
our practices and edit this document accordingly.  It's just a guideline document.  (Credit
to Thömas on this good point)
>> 
>> ~ David

Mime
View raw message