Commit / Code Review Policy

classic Classic list List threaded Threaded
35 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Commit / Code Review Policy

david.w.smiley@gmail.com
Last Wednesday at a Solr committers meeting, there was general agreement in attendance to raise the bar for commit permission to require another's consent, which might not have entailed a review of the code.  I volunteered to draft a proposal.  Other things distracted me but I'm finally thinking of this task now.  *This email is NOT the proposal*. 

I was about to write something from scratch when I discovered we already have some internal documentation on a commit policy that is both reasonably well written/composed and the actual policy/information is pretty good -- kudos to the mystery author!


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.

I plan to copy this to Lucene, mark as PROPOSAL and then make some large edits.  The diff will probably be kinda unrecognizable despite it being in nice shape now.  A "Commit Policy" is more broad that a "Code Review Policy"; it could cover a variety of things.  For example when to commit without even filing a JIRA issue, which I think is worth mentioning.  It should probably also cover Git considerations like merge vs rebase, and multiple commits vs squashing.  Maybe we should also cover when to bother adding to CHANGES.txt and "via"?  Probably commit message requirements.  Snowballing scope :-). Probably not JIRA metadata as it's not part of the commit to be part of a commit policy, but _somewhere_ that's needed.  I'm not sure I want to  sign up for all that now but at least for the code review subject.

~ David Smiley
Apache Lucene/Solr Search Developer
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Jan Høydahl / Cominvent
Mystery authors seem to be

This should probably be part of the new Dev Guide? But go ahead draft in in Confluence first!

--
Jan Høydahl, search solution architect
Cominvent AS - www.cominvent.com

26. nov. 2019 kl. 06:34 skrev David Smiley <[hidden email]>:

Last Wednesday at a Solr committers meeting, there was general agreement in attendance to raise the bar for commit permission to require another's consent, which might not have entailed a review of the code.  I volunteered to draft a proposal.  Other things distracted me but I'm finally thinking of this task now.  *This email is NOT the proposal*. 

I was about to write something from scratch when I discovered we already have some internal documentation on a commit policy that is both reasonably well written/composed and the actual policy/information is pretty good -- kudos to the mystery author!


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.

I plan to copy this to Lucene, mark as PROPOSAL and then make some large edits.  The diff will probably be kinda unrecognizable despite it being in nice shape now.  A "Commit Policy" is more broad that a "Code Review Policy"; it could cover a variety of things.  For example when to commit without even filing a JIRA issue, which I think is worth mentioning.  It should probably also cover Git considerations like merge vs rebase, and multiple commits vs squashing.  Maybe we should also cover when to bother adding to CHANGES.txt and "via"?  Probably commit message requirements.  Snowballing scope :-). Probably not JIRA metadata as it's not part of the commit to be part of a commit policy, but _somewhere_ that's needed.  I'm not sure I want to  sign up for all that now but at least for the code review subject.

~ David Smiley
Apache Lucene/Solr Search Developer

Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Adrien Grand
In reply to this post by david.w.smiley@gmail.com
This document looks reasonable to me and a good description of the way
changes get merged today. Something it says between the lines and that
is the most important bit to me is that this isn't really a policy but
rather a set of guidelines, and that we trust each other to do the
right thing. Maybe we could better reflect this in the name, e.g.
"Commit/Merging guidelines".

On Tue, Nov 26, 2019 at 6:34 AM David Smiley <[hidden email]> wrote:

>
> Last Wednesday at a Solr committers meeting, there was general agreement in attendance to raise the bar for commit permission to require another's consent, which might not have entailed a review of the code.  I volunteered to draft a proposal.  Other things distracted me but I'm finally thinking of this task now.  *This email is NOT the proposal*.
>
> I was about to write something from scratch when I discovered we already have some internal documentation on a commit policy that is both reasonably well written/composed and the actual policy/information is pretty good -- kudos to the mystery author!
>
> https://cwiki.apache.org/confluence/display/SOLR/CommitPolicy
>
> 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.
>
> I plan to copy this to Lucene, mark as PROPOSAL and then make some large edits.  The diff will probably be kinda unrecognizable despite it being in nice shape now.  A "Commit Policy" is more broad that a "Code Review Policy"; it could cover a variety of things.  For example when to commit without even filing a JIRA issue, which I think is worth mentioning.  It should probably also cover Git considerations like merge vs rebase, and multiple commits vs squashing.  Maybe we should also cover when to bother adding to CHANGES.txt and "via"?  Probably commit message requirements.  Snowballing scope :-). Probably not JIRA metadata as it's not part of the commit to be part of a commit policy, but _somewhere_ that's needed.  I'm not sure I want to  sign up for all that now but at least for the code review subject.
>
> ~ David Smiley
> Apache Lucene/Solr Search Developer
> http://www.linkedin.com/in/davidwsmiley



--
Adrien

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Atri Sharma-3
+1

I am generally wary of such proposals since they tend to impose hard processes in the places where trust should be dominant instead.

Apart from that, LGTM

On Tue, 26 Nov 2019 at 14:46, Adrien Grand <[hidden email]> wrote:
This document looks reasonable to me and a good description of the way
changes get merged today. Something it says between the lines and that
is the most important bit to me is that this isn't really a policy but
rather a set of guidelines, and that we trust each other to do the
right thing. Maybe we could better reflect this in the name, e.g.
"Commit/Merging guidelines".

On Tue, Nov 26, 2019 at 6:34 AM David Smiley <[hidden email]> wrote:
>
> Last Wednesday at a Solr committers meeting, there was general agreement in attendance to raise the bar for commit permission to require another's consent, which might not have entailed a review of the code.  I volunteered to draft a proposal.  Other things distracted me but I'm finally thinking of this task now.  *This email is NOT the proposal*.
>
> I was about to write something from scratch when I discovered we already have some internal documentation on a commit policy that is both reasonably well written/composed and the actual policy/information is pretty good -- kudos to the mystery author!
>
> https://cwiki.apache.org/confluence/display/SOLR/CommitPolicy
>
> 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.
>
> I plan to copy this to Lucene, mark as PROPOSAL and then make some large edits.  The diff will probably be kinda unrecognizable despite it being in nice shape now.  A "Commit Policy" is more broad that a "Code Review Policy"; it could cover a variety of things.  For example when to commit without even filing a JIRA issue, which I think is worth mentioning.  It should probably also cover Git considerations like merge vs rebase, and multiple commits vs squashing.  Maybe we should also cover when to bother adding to CHANGES.txt and "via"?  Probably commit message requirements.  Snowballing scope :-). Probably not JIRA metadata as it's not part of the commit to be part of a commit policy, but _somewhere_ that's needed.  I'm not sure I want to  sign up for all that now but at least for the code review subject.
>
> ~ David Smiley
> Apache Lucene/Solr Search Developer
> http://www.linkedin.com/in/davidwsmiley



--
Adrien

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

--
Regards,

Atri
Apache Concerted
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Joel Bernstein
I think more is needed going forward.

What I would like to see is an explicit "risks" section of the jira that shows the committer has thought about the different risks to the system before committing code that effects the core. The committer should take time to understand what part of the system might be effected. This will do two things:

1) Force the committer to think more about how there change affects the rest of the system.
2) Helps other committers understand the risks so that there is more incentive to get involved and test.







On Tue, Nov 26, 2019 at 4:26 AM Atri Sharma <[hidden email]> wrote:
+1

I am generally wary of such proposals since they tend to impose hard processes in the places where trust should be dominant instead.

Apart from that, LGTM

On Tue, 26 Nov 2019 at 14:46, Adrien Grand <[hidden email]> wrote:
This document looks reasonable to me and a good description of the way
changes get merged today. Something it says between the lines and that
is the most important bit to me is that this isn't really a policy but
rather a set of guidelines, and that we trust each other to do the
right thing. Maybe we could better reflect this in the name, e.g.
"Commit/Merging guidelines".

On Tue, Nov 26, 2019 at 6:34 AM David Smiley <[hidden email]> wrote:
>
> Last Wednesday at a Solr committers meeting, there was general agreement in attendance to raise the bar for commit permission to require another's consent, which might not have entailed a review of the code.  I volunteered to draft a proposal.  Other things distracted me but I'm finally thinking of this task now.  *This email is NOT the proposal*.
>
> I was about to write something from scratch when I discovered we already have some internal documentation on a commit policy that is both reasonably well written/composed and the actual policy/information is pretty good -- kudos to the mystery author!
>
> https://cwiki.apache.org/confluence/display/SOLR/CommitPolicy
>
> 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.
>
> I plan to copy this to Lucene, mark as PROPOSAL and then make some large edits.  The diff will probably be kinda unrecognizable despite it being in nice shape now.  A "Commit Policy" is more broad that a "Code Review Policy"; it could cover a variety of things.  For example when to commit without even filing a JIRA issue, which I think is worth mentioning.  It should probably also cover Git considerations like merge vs rebase, and multiple commits vs squashing.  Maybe we should also cover when to bother adding to CHANGES.txt and "via"?  Probably commit message requirements.  Snowballing scope :-). Probably not JIRA metadata as it's not part of the commit to be part of a commit policy, but _somewhere_ that's needed.  I'm not sure I want to  sign up for all that now but at least for the code review subject.
>
> ~ David Smiley
> Apache Lucene/Solr Search Developer
> http://www.linkedin.com/in/davidwsmiley



--
Adrien

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

--
Regards,

Atri
Apache Concerted
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Michael Sokolov-4
In reply to this post by david.w.smiley@gmail.com
I think the recent replies seem to be in response to the existing
document (CommitPolicy), but what David is proposing is to completely
rewrite that document into something (his words) "unrecognizable"? So
I'll hold off on commenting until we've seen the actual proposal?

-Mike

On Tue, Nov 26, 2019 at 12:35 AM David Smiley <[hidden email]> wrote:

>
> Last Wednesday at a Solr committers meeting, there was general agreement in attendance to raise the bar for commit permission to require another's consent, which might not have entailed a review of the code.  I volunteered to draft a proposal.  Other things distracted me but I'm finally thinking of this task now.  *This email is NOT the proposal*.
>
> I was about to write something from scratch when I discovered we already have some internal documentation on a commit policy that is both reasonably well written/composed and the actual policy/information is pretty good -- kudos to the mystery author!
>
> https://cwiki.apache.org/confluence/display/SOLR/CommitPolicy
>
> 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.
>
> I plan to copy this to Lucene, mark as PROPOSAL and then make some large edits.  The diff will probably be kinda unrecognizable despite it being in nice shape now.  A "Commit Policy" is more broad that a "Code Review Policy"; it could cover a variety of things.  For example when to commit without even filing a JIRA issue, which I think is worth mentioning.  It should probably also cover Git considerations like merge vs rebase, and multiple commits vs squashing.  Maybe we should also cover when to bother adding to CHANGES.txt and "via"?  Probably commit message requirements.  Snowballing scope :-). Probably not JIRA metadata as it's not part of the commit to be part of a commit policy, but _somewhere_ that's needed.  I'm not sure I want to  sign up for all that now but at least for the code review subject.
>
> ~ David Smiley
> Apache Lucene/Solr Search Developer
> http://www.linkedin.com/in/davidwsmiley

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

david.w.smiley@gmail.com
The commit policy / guideline document is basically 95% there and I don't want to wait longer to get input.

If you log-in, you can comment on the document in-line as Jan has already done.  Such feedback is good for details.  For more substantive or high level feedback, this email thread probably makes more sense.

The policy/guideline document insists on reviews but gives broad exceptions for reviews and defines a very low bar for reviews -- basically mere "approval" from anyone and that didn't necessarily look at the code.  Yet this is a higher bar than today.

Also, I hope this is not controversial but I want the same definition of minor/trival matters to be used for (A) when a JIRA issue is not needed either, and (B) not bothering with a CHANGES.txt entry.  I observe that today we seemingly have a JIRA issue for everything, and I find that onerous and is yet another barrier for contributors of such small matters.  For example https://issues.apache.org/jira/browse/SOLR-13926 which just adds javadocs.  Also I think we add too many items to CHANGES.txt... lots of people read this and it's a collective waste of our time IMO to mention that some test was fixed.

All feedback is very welcome!

~ David
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Bruno Roustant
I like this new version. This clarifies the review, commit and CHANGES. As a beginner in this process, it helps.

I appreciate the idea to have a "risk" section where we could list and say a few words about some risky areas so that the contributor can announce they might be impacted in reviews.

Le ven. 29 nov. 2019 à 16:06, David Smiley <[hidden email]> a écrit :
The commit policy / guideline document is basically 95% there and I don't want to wait longer to get input.

If you log-in, you can comment on the document in-line as Jan has already done.  Such feedback is good for details.  For more substantive or high level feedback, this email thread probably makes more sense.

The policy/guideline document insists on reviews but gives broad exceptions for reviews and defines a very low bar for reviews -- basically mere "approval" from anyone and that didn't necessarily look at the code.  Yet this is a higher bar than today.

Also, I hope this is not controversial but I want the same definition of minor/trival matters to be used for (A) when a JIRA issue is not needed either, and (B) not bothering with a CHANGES.txt entry.  I observe that today we seemingly have a JIRA issue for everything, and I find that onerous and is yet another barrier for contributors of such small matters.  For example https://issues.apache.org/jira/browse/SOLR-13926 which just adds javadocs.  Also I think we add too many items to CHANGES.txt... lots of people read this and it's a collective waste of our time IMO to mention that some test was fixed.

All feedback is very welcome!

~ David
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Robert Muir
In reply to this post by david.w.smiley@gmail.com


On Tue, Nov 26, 2019 at 12:34 AM David Smiley <[hidden email]> 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 don't think changing things to review-then-commit will help.

Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Shawn Heisey-2
On 11/30/2019 7:39 AM, Robert Muir wrote:
> -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 don't think changing things to review-then-commit will help.

I did attend the meeting, and I think committers who concentrate on
Lucene should not be discouraged from attending any similar future
meetings.  Discussions in a meeting about Solr are not very likely to
dive down that far into the inner workings, but even if they don't, some
Lucene people might find the the talk about Solr to be interesting, and
having their perspective available would not be a bad thing.

I'm very wary of making an official change to review-then-commit.  I
fully support the ideas that went into the proposal, but I think making
it mandatory for all commits is going to really slow things down and
cause some problems.  It's not the way most Apache projects work, and it
makes it a LOT harder to do quick sanity edits like fixing spelling errors.

As much as I really like the idea of more frequent reviews, I think that
review requirements should be informal.

Most of the time a committer will know whether the changes they are
working on fall into a "major" or "minor" category.  When it's leaning
more towards major, I think most of us will agree that a few extra
eyeballs looking for gotchas is a really good idea.  TLDR note:  The
number of lines in a change is sometimes NOT an indicator of how major
the change is.

I certainly want to seek a "looks good to me" on changes I make which
have any significant impact.  For some of the ideas I have, if those
ever reach the implementation phase, I think I'd want even more
assurance that I'm not doing it wrong.  I can pledge that I will seek
review for non-trivial changes.  I wonder if there is wording we can add
to any official project rulebook to make such a thing mandatory, without
a full switch.

I think that a full switch to review-then-commit (RTC) would be the
wrong thing to do.  I think it would lead to a lot of dissent within the
project, encouraging rivalries and cliques within the project.

If RTC is preferred by a significant majority, I will work within that
paradigm ... but I think that it would be a short-lived experiment and
we would be back to CTR pretty quickly.

Is it possible to vote -0.5 instead of -1?  I don't think it is.

Thanks,
Shawn

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Jason Gerlowski
> and now its turned around as if its consensus everywhere

David didn't say anything about consensus everywhere.  He mentioned
pretty clearly that the agreement was only "in attendance", and that
this thread was a precursor for putting together a proposal to test
the waters further.  That all seems pretty in line with the Apache
process for feeling out/testing/etc. consensus, though maybe there's a
nuance I'm missing?

Jason

On Sat, Nov 30, 2019 at 3:27 PM Shawn Heisey <[hidden email]> wrote:

>
> On 11/30/2019 7:39 AM, Robert Muir wrote:
> > -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 don't think changing things to review-then-commit will help.
>
> I did attend the meeting, and I think committers who concentrate on
> Lucene should not be discouraged from attending any similar future
> meetings.  Discussions in a meeting about Solr are not very likely to
> dive down that far into the inner workings, but even if they don't, some
> Lucene people might find the the talk about Solr to be interesting, and
> having their perspective available would not be a bad thing.
>
> I'm very wary of making an official change to review-then-commit.  I
> fully support the ideas that went into the proposal, but I think making
> it mandatory for all commits is going to really slow things down and
> cause some problems.  It's not the way most Apache projects work, and it
> makes it a LOT harder to do quick sanity edits like fixing spelling errors.
>
> As much as I really like the idea of more frequent reviews, I think that
> review requirements should be informal.
>
> Most of the time a committer will know whether the changes they are
> working on fall into a "major" or "minor" category.  When it's leaning
> more towards major, I think most of us will agree that a few extra
> eyeballs looking for gotchas is a really good idea.  TLDR note:  The
> number of lines in a change is sometimes NOT an indicator of how major
> the change is.
>
> I certainly want to seek a "looks good to me" on changes I make which
> have any significant impact.  For some of the ideas I have, if those
> ever reach the implementation phase, I think I'd want even more
> assurance that I'm not doing it wrong.  I can pledge that I will seek
> review for non-trivial changes.  I wonder if there is wording we can add
> to any official project rulebook to make such a thing mandatory, without
> a full switch.
>
> I think that a full switch to review-then-commit (RTC) would be the
> wrong thing to do.  I think it would lead to a lot of dissent within the
> project, encouraging rivalries and cliques within the project.
>
> If RTC is preferred by a significant majority, I will work within that
> paradigm ... but I think that it would be a short-lived experiment and
> we would be back to CTR pretty quickly.
>
> Is it possible to vote -0.5 instead of -1?  I don't think it is.
>
> Thanks,
> Shawn
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

david.w.smiley@gmail.com
In reply to this post by Robert Muir
Hello Rob, 

On Sat, Nov 30, 2019 at 9:39 AM Robert Muir <[hidden email]> wrote:
On Tue, Nov 26, 2019 at 12:34 AM David Smiley <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Robert Muir
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 <[hidden email]> wrote:
Hello Rob, 

On Sat, Nov 30, 2019 at 9:39 AM Robert Muir <[hidden email]> wrote:
On Tue, Nov 26, 2019 at 12:34 AM David Smiley <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Jan Høydahl / Cominvent
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 <[hidden email]>:


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 <[hidden email]> wrote:
Hello Rob, 

On Sat, Nov 30, 2019 at 9:39 AM Robert Muir <[hidden email]> wrote:
On Tue, Nov 26, 2019 at 12:34 AM David Smiley <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Robert Muir


On Mon, Dec 2, 2019 at 3:49 AM Jan Høydahl <[hidden email]> wrote:


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. 


Then the number one thing to do is fix the tests, so that they can be trusted and leaned on more to detect these bugs. As someone who wrestled with these tests recently... in all honesty something drastic needs to happen (e.g. throwing them all out and starting over).

People are always going to make mistakes. These mistakes will sometimes slip past reviewers, too. No matter how much process and time you throw at it.

But i guess "policy" is sexier to work on than tests.
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Robert Muir
In reply to this post by Jan Høydahl / Cominvent


On Mon, Dec 2, 2019 at 3:49 AM Jan Høydahl <[hidden email]> wrote:
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.

Then change the document's name to be Recommendation instead of Policy!
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Ishan Chattopadhyaya
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?

+1 Robert, totally agree. RCE etc should be absolutely top priority. Thanks a ton for tackling this. Breaking functionality (not deliberately of course) is better than having RCEs in a release. IOW, it can't get worse.

On Mon, 2 Dec, 2019, 3:03 PM Robert Muir, <[hidden email]> wrote:


On Mon, Dec 2, 2019 at 3:49 AM Jan Høydahl <[hidden email]> wrote:
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.

Then change the document's name to be Recommendation instead of Policy!
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Tomás Fernández Löbbe
Thank you for putting this up, David. While I see Rob’s points, I agree with the proposal in general, and as you and Jan said, this is not too far from what happens today, at least in Lucene-world. 

Then change the document's name to be Recommendation instead of Policy!

Maybe guidelines? The doc itself says so, so maybe the name should reflect the intention.

there is no "silence is consensus"

Good point, maybe we should include something about this too. While hopefully this doesn’t happen much, it doesn’t make sense to stall work for weeks after putting up a final patch. Again, I think the doc’s intention is to be a guideline, but if there are exceptions, so be it.

People are always going to make mistakes. These mistakes will sometimes slip past reviewers, too. No matter how much process and time you throw at it. 

Not perfect, true, but in my experience a lot is caught in reviews. 

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.

Makes sense, maybe we can address that later.

On Mon, Dec 2, 2019 at 8:00 AM Ishan Chattopadhyaya <[hidden email]> wrote:
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?

+1 Robert, totally agree. RCE etc should be absolutely top priority. Thanks a ton for tackling this. Breaking functionality (not deliberately of course) is better than having RCEs in a release. IOW, it can't get worse.

On Mon, 2 Dec, 2019, 3:03 PM Robert Muir, <[hidden email]> wrote:


On Mon, Dec 2, 2019 at 3:49 AM Jan Høydahl <[hidden email]> wrote:
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.

Then change the document's name to be Recommendation instead of Policy!
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Gus Heck
In reply to this post by Ishan Chattopadhyaya
Some thoughts from reading the doc and this thread

  1. This doesn't sound like a huge change, mostly a change in tone since nearly everything in it is a judgement call. 
  2. I agree that the word policy seems iffy. It has a "consequences for violation" tone to it, and without some sort of "what happens if you break it" it's really just a recommendation anyway. Maybe call it "Commit Process Recommendations"
  3. It would be nice if JIRA had a (optional!) PATCH_REVIEW status (down stream from PATCH_AVAILABLE and it's infrastructure, but not requiring PATCH_AVAILABLE as a prior step) that could be hooked up to send a mail with an easily filterable subject line to individuals that have (somehow) nominated themselves as interested in the associated Jira Component
  4. If we add the status, we should formalize the expected time period that reviewers will have to respond, even if it's just with "I want to review this but I can't get to it till Thursday..." (I'm thinking 1 week). This timeline would only apply to issues that use this status, timelines for small stuff, or major changes etc as per the recommendations... This is just an idea to help folks who want a review to connect with and get the attention of folks who care particularly deeply about a feature area. 

On Mon, Dec 2, 2019 at 6:00 AM Ishan Chattopadhyaya <[hidden email]> wrote:
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?

+1 Robert, totally agree. RCE etc should be absolutely top priority. Thanks a ton for tackling this. Breaking functionality (not deliberately of course) is better than having RCEs in a release. IOW, it can't get worse.

On Mon, 2 Dec, 2019, 3:03 PM Robert Muir, <[hidden email]> wrote:


On Mon, Dec 2, 2019 at 3:49 AM Jan Høydahl <[hidden email]> wrote:
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.

Then change the document's name to be Recommendation instead of Policy!


--
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Anshum Gupta
In reply to this post by david.w.smiley@gmail.com
Thanks David, and every one else here.

Let's not call this policy, as 1. there are folks other than me who think that's too strong a word for what we are trying to do here, 2. this has "rules" that we will have to ignore based on the circumstance. these rules should instead be recommendations, and the policy be either what Robert or Tomas suggested i.e. Recommendation/Guidelines.

I feel that the document might be read in a different context by people who weren't in the meeting. As far as I understand, the intention here is to have better quality commits, with involvement of more than one person, especially when the changes are large and affect the core/API.

There are points around adding tests, which also directly are about just improving code quality, something that has led to the failing tests and other issues that have been spoken about in the recent past.

+1 on adding something explicitly about "silence is consensus". 
There are certain parts of the code that aren't worked by more than a couple of people and in that case, these points will have to be just interpreted based on the situation.

Overall, I really think it will not only be awesome  but is critical to have more people reviewing code and for larger changes to be discussed instead of being pushed in.


On Tue, Nov 26, 2019 at 11:04 AM David Smiley <[hidden email]> wrote:
Last Wednesday at a Solr committers meeting, there was general agreement in attendance to raise the bar for commit permission to require another's consent, which might not have entailed a review of the code.  I volunteered to draft a proposal.  Other things distracted me but I'm finally thinking of this task now.  *This email is NOT the proposal*. 

I was about to write something from scratch when I discovered we already have some internal documentation on a commit policy that is both reasonably well written/composed and the actual policy/information is pretty good -- kudos to the mystery author!


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.

I plan to copy this to Lucene, mark as PROPOSAL and then make some large edits.  The diff will probably be kinda unrecognizable despite it being in nice shape now.  A "Commit Policy" is more broad that a "Code Review Policy"; it could cover a variety of things.  For example when to commit without even filing a JIRA issue, which I think is worth mentioning.  It should probably also cover Git considerations like merge vs rebase, and multiple commits vs squashing.  Maybe we should also cover when to bother adding to CHANGES.txt and "via"?  Probably commit message requirements.  Snowballing scope :-). Probably not JIRA metadata as it's not part of the commit to be part of a commit policy, but _somewhere_ that's needed.  I'm not sure I want to  sign up for all that now but at least for the code review subject.

~ David Smiley
Apache Lucene/Solr Search Developer


--
Anshum Gupta
12