You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by David Smiley <da...@gmail.com> on 2019/11/26 05:34:33 UTC
Commit / Code Review Policy
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
Re: Commit / Code Review Policy
Posted by Jan Høydahl <ja...@cominvent.com>.
Mystery authors seem to be
- Hoss https://web.archive.org/web/20071011105632/http://wiki.apache.org/solr/CommitPolicy <https://web.archive.org/web/20071011105632/http://wiki.apache.org/solr/CommitPolicy>
- Grant https://web.archive.org/web/20090504015648/http://wiki.apache.org/solr/CommitPolicy <https://web.archive.org/web/20090504015648/http://wiki.apache.org/solr/CommitPolicy>
- Myself https://web.archive.org/web/20111119003001/http://wiki.apache.org/solr/CommitPolicy <https://web.archive.org/web/20111119003001/http://wiki.apache.org/solr/CommitPolicy> :)
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 <da...@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!
>
> https://cwiki.apache.org/confluence/display/SOLR/CommitPolicy <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 <http://www.linkedin.com/in/davidwsmiley>
Re: Commit / Code Review Policy
Posted by Joel Bernstein <jo...@gmail.com>.
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.
Joel Bernstein
http://joelsolr.blogspot.com/
On Tue, Nov 26, 2019 at 4:26 AM Atri Sharma <at...@apache.org> 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 <jp...@gmail.com> 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 <da...@gmail.com>
>> 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: dev-unsubscribe@lucene.apache.org
>> For additional commands, e-mail: dev-help@lucene.apache.org
>>
>> --
> Regards,
>
> Atri
> Apache Concerted
>
Re: Commit / Code Review Policy
Posted by Atri Sharma <at...@apache.org>.
+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 <jp...@gmail.com> 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 <da...@gmail.com>
> 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: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>
> --
Regards,
Atri
Apache Concerted
Re: Commit / Code Review Policy
Posted by Adrien Grand <jp...@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 <da...@gmail.com> 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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: Commit / Code Review Policy
Posted by Robert Muir <rc...@gmail.com>.
On Mon, Dec 2, 2019 at 3:49 AM Jan Høydahl <ja...@cominvent.com> 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.
Re: Commit / Code Review Policy
Posted by Tomás Fernández Löbbe <to...@gmail.com>.
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 <
ichattopadhyaya@gmail.com> 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, <rc...@gmail.com> wrote:
>
>>
>>
>> On Mon, Dec 2, 2019 at 3:49 AM Jan Høydahl <ja...@cominvent.com> 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!
>>
>
Re: Commit / Code Review Policy
Posted by Gus Heck <gu...@gmail.com>.
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 <
ichattopadhyaya@gmail.com> 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, <rc...@gmail.com> wrote:
>
>>
>>
>> On Mon, Dec 2, 2019 at 3:49 AM Jan Høydahl <ja...@cominvent.com> 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!
>>
>
--
http://www.needhamsoftware.com (work)
http://www.the111shift.com (play)
Re: Commit / Code Review Policy
Posted by Ishan Chattopadhyaya <ic...@gmail.com>.
> 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, <rc...@gmail.com> wrote:
>
>
> On Mon, Dec 2, 2019 at 3:49 AM Jan Høydahl <ja...@cominvent.com> 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!
>
Re: Commit / Code Review Policy
Posted by Robert Muir <rc...@gmail.com>.
On Mon, Dec 2, 2019 at 3:49 AM Jan Høydahl <ja...@cominvent.com> 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!
Re: Commit / Code Review Policy
Posted by Jan Høydahl <ja...@cominvent.com>.
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 <rc...@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 <da...@gmail.com> wrote:
>> Hello Rob,
>>
>> On Sat, Nov 30, 2019 at 9:39 AM Robert Muir <rc...@gmail.com> wrote:
>>>> On Tue, Nov 26, 2019 at 12:34 AM David Smiley <da...@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
Re: Commit / Code Review Policy
Posted by Robert Muir <rc...@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 <da...@gmail.com>
wrote:
> Hello Rob,
>
> On Sat, Nov 30, 2019 at 9:39 AM Robert Muir <rc...@gmail.com> wrote:
>
>> On Tue, Nov 26, 2019 at 12:34 AM David Smiley <da...@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
>
Re: Commit / Code Review Policy
Posted by David Smiley <da...@gmail.com>.
Hello Rob,
On Sat, Nov 30, 2019 at 9:39 AM Robert Muir <rc...@gmail.com> wrote:
> On Tue, Nov 26, 2019 at 12:34 AM David Smiley <da...@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
Re: Commit / Code Review Policy
Posted by Jason Gerlowski <ge...@gmail.com>.
> 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 <ap...@elyograg.org> 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: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: Commit / Code Review Policy
Posted by Shawn Heisey <ap...@elyograg.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: Commit / Code Review Policy
Posted by Robert Muir <rc...@gmail.com>.
On Tue, Nov 26, 2019 at 12:34 AM David Smiley <da...@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 don't think changing things to review-then-commit will help.
Re: Commit / Code Review Policy
Posted by Bruno Roustant <br...@gmail.com>.
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 <da...@gmail.com> a
écrit :
> The commit policy / guideline document is basically 95% there and I don't
> want to wait longer to get input.
> https://cwiki.apache.org/confluence/display/LUCENE/Commit+Policy+-+DRAFT
>
> 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
>
Re: Commit / Code Review Policy
Posted by David Smiley <da...@gmail.com>.
The commit policy / guideline document is basically 95% there and I don't
want to wait longer to get input.
https://cwiki.apache.org/confluence/display/LUCENE/Commit+Policy+-+DRAFT
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
Re: Commit / Code Review Policy
Posted by Michael Sokolov <ms...@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 <da...@gmail.com> 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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: Commit / Code Review Policy
Posted by David Smiley <da...@gmail.com>.
It's not an either-or. Yes Solr has too many old/deprecated items that
aren't getting removed. Yes also many features ought to be in plugins that
are opt-in (HDFS). Yes also many of us want to raise the peer-review bar
for all the reasons peer reviews bring about better quality software.
BTW I don't think some sort of feature/improvement block is something we
can agree to, but it's off-topic any way.
Before raising other subjects, are you or anyone else uncomfortable with
the proposed guidelines -- particularly anything *not* demarcated
with [PENDING DISCUSSION] ? If so please respond with a suggestion on how
I can improve it.
~ David Smiley
Apache Lucene/Solr Search Developer
http://www.linkedin.com/in/davidwsmiley
On Wed, Dec 4, 2019 at 8:12 AM Robert Muir <rc...@gmail.com> wrote:
>
> On Tue, Dec 3, 2019 at 3:02 PM Doug Turnbull <
> dturnbull@opensourceconnections.com> wrote:
>
>> As more of a practioner/user of Solr, I would second a desire to have
>> more review involved in what gets added. I am sometimes surprised at
>> features that have gotten added with minimal review that feel fairly
>> experimental or impact stability. I don't think it's anything against the
>> people working on the features, as a sometimes contributor, I too have not
>> fully thought out all the implications, big and small, of my desired
>> changes. I have been rather impressed how much my contribution has improved
>> when a committer (namely Mr. Smiley, who is an incredible human being) has
>> helped review & shephard the change.
>>
>
> I don't think this has anything to do with code review: it has to do with
> people just piling in features, but not taking the time to do any
> janitorial work or remove old features that shouldn't be there anymore (I
> AM LOOKING AT YOU HDFS)
>
> Solr really must *remove features* from time to time, and then refactor
> the code to remove any hacks those features brought in, like a normal
> software project.
>
> So instead of adding a bunch of policy, it might be a better idea to
> directly attack the problem of piling up features, maybe declare a
> moratorium on new features for a while, remove some outdated ones, and do
> some long overdue cleanup.
>
Re: Commit / Code Review Policy
Posted by Robert Muir <rc...@gmail.com>.
On Tue, Dec 3, 2019 at 3:02 PM Doug Turnbull <
dturnbull@opensourceconnections.com> wrote:
> As more of a practioner/user of Solr, I would second a desire to have more
> review involved in what gets added. I am sometimes surprised at features
> that have gotten added with minimal review that feel fairly experimental or
> impact stability. I don't think it's anything against the people working on
> the features, as a sometimes contributor, I too have not fully thought out
> all the implications, big and small, of my desired changes. I have been
> rather impressed how much my contribution has improved when a
> committer (namely Mr. Smiley, who is an incredible human being) has helped
> review & shephard the change.
>
I don't think this has anything to do with code review: it has to do with
people just piling in features, but not taking the time to do any
janitorial work or remove old features that shouldn't be there anymore (I
AM LOOKING AT YOU HDFS)
Solr really must *remove features* from time to time, and then refactor the
code to remove any hacks those features brought in, like a normal software
project.
So instead of adding a bunch of policy, it might be a better idea to
directly attack the problem of piling up features, maybe declare a
moratorium on new features for a while, remove some outdated ones, and do
some long overdue cleanup.
Re: Commit / Code Review Policy
Posted by Doug Turnbull <dt...@opensourceconnections.com>.
As more of a practioner/user of Solr, I would second a desire to have more
review involved in what gets added. I am sometimes surprised at features
that have gotten added with minimal review that feel fairly experimental or
impact stability. I don't think it's anything against the people working on
the features, as a sometimes contributor, I too have not fully thought out
all the implications, big and small, of my desired changes. I have been
rather impressed how much my contribution has improved when a
committer (namely Mr. Smiley, who is an incredible human being) has helped
review & shephard the change.
I think the frustration waiting months for a review echoes earlier
frustrations on this thread (and I've heard from many people) that Solr is
not friendly/open to contributors. I know some folks who have put a great
deal of time into contributions, including reimplemting patches multiple
times for new Solr versions, to see them languish. So perhaps, if it is
true that a committer needs to wait for months for a +1, then that to me
signals a problem that the community is not focused enough on
shepharding/giving feedback to other people's work into Solr in a cohesive
way that maintains some stability and a unifying vision moving forward.
Just my 2 cents, I'm just giving my subjective perception of what I
see/hear when I go to clients and talk to them about Solr. This is often
how Solr is perceived that you have to scream & hollar and really tackle a
committer to get something in
Conversely, if I submit a PR to Elasticsearch, it probably won't get
approved, but at least I get a response and a swift determination quickly :)
Best
-Doug
On Tue, Dec 3, 2019 at 2:42 PM Mike Drob <md...@apache.org> 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.
> 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 <da...@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, <rc...@gmail.com> wrote:
>>>
>>>>
>>>>
>>>> On Mon, Dec 2, 2019 at 3:33 PM David Smiley <da...@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 :)
>>>>
>>>>
>>>>
--
*Doug Turnbull **| CTO* | OpenSource Connections
<http://opensourceconnections.com>, LLC | 240.476.9983
Author: Relevant Search <http://manning.com/turnbull>
This e-mail and all contents, including attachments, is considered to be
Company Confidential unless explicitly stated otherwise, regardless
of whether attachments are marked as such.
Re: Commit / Code Review Policy
Posted by David Smiley <da...@gmail.com>.
On Wed, Mar 4, 2020 at 8:36 AM Daniel Ruggeri <dr...@apache.org> wrote:
> I definitely reviewed the email thread, replies, and Confluence wiki
> document. In my interpretation of the discussion, a few things stood out:
> - There were some synchronous discussions that occurred off the list,
> which can lead to accidentally excluding important community voices from
> the conversation. By bringing the discussion back to the list before
> attempting to make a decision, the critical levels of transparency are
> maintained. This is definitely A Good Thing.
>
I agree 100% in what you said. I thought about this principle as I
conducted my business (off-list and on-list). I'm frustrated that not
everyone agreed; I really do care! If we are so fearful of crossing a line
here, then we end up limiting ourselves to excluding opportunities to see
each other and hear each other's voices, and that would be shame! A lot
can be lost in purely text chat in ascertaining emotion / intent. Seeing
and hearing each other can help diffuse possible conflict and furthermore
it enriches our relationships in subtle ways. Do you agree? Do other
projects have written guidelines that we can adopt to state this more
concretely so we don't cross a line?
> - I perceived a lot of constructive feedback on the initial draft which
> appears (to me) to have brought the draft much closer to a document that
> represents consensus. Said another way, I don't detect conflict per se
> (please correct me if I'm wrong)
>
I agree; it was all constructive.
> - I think the feedback (and subsequent incorporation of that feedback into
> the guidelines) about exception cases like doc fixes and "small" changes is
> brilliant. On the httpd project, as a parallel example, we have a RTC
> policy for release branches except for docs - which are CTR. Indeed, a
> "docs committer" is the same as an "httpd committer"... we don't separate
> privileges because this social contract has worked well for 2 decades.
>
Thanks! Documenting this is important to me as too often I see too much
ceremony for such trivial stuff which in turn leads to reduced
contributions from me because I don't want to go through that ceremony when
I don't think it's warranted.
> - The current content of the document seems reasonable given the current
> environment. One thing that is unclear to me based on a quick browsing of
> the docs is what the branching strategy is for the project. What I gather
> is that the master branch is the release branch and features are added to
> master or merged in from short-lived feature branches. It'd be helpful to
> clarify this.
>
True; we should probably state the strategy _somewhere_; maybe this
document is where. Although such info wouldn't really be
guidance/advice so I'm not sure it goes on the document. We branch off
from our main/dev branches to do releases, and so I wouldn't call "master"
and "branch_8x" release branches since we do have actual release branches
created at feature-freeze time.
> - The proposed document draws additional parallels to things that work for
> httpd. Often times, we will share patches to the list for feedback before
> commits (similar to the Jira tickets proposed). We also operate trunk as
> CTR, but require 3 +1 backport votes to the long-lived release branches.
> This proposal has a similar net effect: bits that get released are
> (generally) intended to be "actively" reviewed before commit. It does fall
> short of requiring a consensus-style vote for release branches as httpd
> does... but that's OK.
>
Hmmm. What we do is have the release manager be a required gatekeeper as
to what may be committed to a release branch (yay/nay decision maker), and
we know basically what the general qualifications are which are repeated at
every feature freeze. This document would be a good place to state them;
good idea.
>
> All told, this proposal seems quite reasonable to me because it has been
> discussed by the community, feedback has been incorporated, and the content
> of the proposal seems totally doable. That said, I'm open to other
> inquiries if there's something an external perspective can provide. How can
> I help?
>
I suppose we're in good shape then. I have one question that you more than
others could give an authoritative answer to: Do projects really choose
RTC: http://www.apache.org/foundation/glossary.html#ReviewThenCommit given
the onerous requirements? It's hard for me to imagine it being workable
unless there were lots of active committers and if the volume of change is
low and if the changes are potentially sensitive (e.g. for encryption). If
the Lucene project hypothetically wanted to insist on at least one +1 from
someone, would we not be able to call this RTC because it doesn't meet the
definition above? I suspect in truth we can define our policy as we wish.
>
> [1] https://whimsy.apache.org/board/minutes/Lucene.html
>
> P.S.
> Nabble, mbox, etc... they're OK, but have you tried Ponymail?
>
> https://lists.apache.org/x/thread.html/770ef83a8ed3a3d5d161c4a2cd812b4bdde47464d439c09ec31164dd@%3Cdev.lucene.apache.org%3E
> I'm a convert, for sure!
Thanks for showing me Ponymail! It's *way* better than whatever that
interface is for mod_mbox URLs. Our project website should replace all
such links to Ponymail.
~ David
Re: Commit / Code Review Policy
Posted by David Smiley <da...@gmail.com>.
(Renamed; no other changes)
https://cwiki.apache.org/confluence/display/LUCENE/Commit+Process+Guidelines
~ David Smiley
Apache Lucene/Solr Search Developer
http://www.linkedin.com/in/davidwsmiley
On Wed, Mar 4, 2020 at 8:36 AM Daniel Ruggeri <dr...@apache.org> wrote:
> Hi, all;
> I could have *sworn* I replied... but could find no record in my sent
> folder when David dutifully followed up with a ping. *facepalm*
>
> Yes, I volunteered to participate and help where I can as a member of The
> ASF board, VP Fundraising, ComDev participant, or whatever hat I might be
> able to bring with me. Of course, I bear no special credentials here... we
> all hang out as volunteers. Reviewing the most recent board report[1], I
> detected:
> > We're discussing how to semi-require code reviews by defining project
> guidelines. We invite a board member to help/advice how to navigate this
> balance.
> So, yeah, here I am :-) My primary interest is to be sure we're
> approaching the discussion with community as our primary lens.
>
> I definitely reviewed the email thread, replies, and Confluence wiki
> document. In my interpretation of the discussion, a few things stood out:
> - There were some synchronous discussions that occurred off the list,
> which can lead to accidentally excluding important community voices from
> the conversation. By bringing the discussion back to the list before
> attempting to make a decision, the critical levels of transparency are
> maintained. This is definitely A Good Thing.
> - I perceived a lot of constructive feedback on the initial draft which
> appears (to me) to have brought the draft much closer to a document that
> represents consensus. Said another way, I don't detect conflict per se
> (please correct me if I'm wrong)
> - I think the feedback (and subsequent incorporation of that feedback into
> the guidelines) about exception cases like doc fixes and "small" changes is
> brilliant. On the httpd project, as a parallel example, we have a RTC
> policy for release branches except for docs - which are CTR. Indeed, a
> "docs committer" is the same as an "httpd committer"... we don't separate
> privileges because this social contract has worked well for 2 decades.
> - The current content of the document seems reasonable given the current
> environment. One thing that is unclear to me based on a quick browsing of
> the docs is what the branching strategy is for the project. What I gather
> is that the master branch is the release branch and features are added to
> master or merged in from short-lived feature branches. It'd be helpful to
> clarify this.
> - The proposed document draws additional parallels to things that work for
> httpd. Often times, we will share patches to the list for feedback before
> commits (similar to the Jira tickets proposed). We also operate trunk as
> CTR, but require 3 +1 backport votes to the long-lived release branches.
> This proposal has a similar net effect: bits that get released are
> (generally) intended to be "actively" reviewed before commit. It does fall
> short of requiring a consensus-style vote for release branches as httpd
> does... but that's OK.
>
> All told, this proposal seems quite reasonable to me because it has been
> discussed by the community, feedback has been incorporated, and the content
> of the proposal seems totally doable. That said, I'm open to other
> inquiries if there's something an external perspective can provide. How can
> I help?
>
>
> [1] https://whimsy.apache.org/board/minutes/Lucene.html
>
> P.S.
> Nabble, mbox, etc... they're OK, but have you tried Ponymail?
>
> https://lists.apache.org/x/thread.html/770ef83a8ed3a3d5d161c4a2cd812b4bdde47464d439c09ec31164dd@%3Cdev.lucene.apache.org%3E
> I'm a convert, for sure!
> --
> Daniel Ruggeri
> Director, VP Fundraising, member, httpd PMC
> The Apache Software Foundation
>
> On February 7, 2020 9:14:37 AM CST, David Smiley <da...@gmail.com>
> wrote:
>>
>> I neglected to try and close up this long thread on the subject of code
>> review guidance. In the project's board report to the ASF, I asked for
>> help; Daniel Ruggeri (an ASF VP) graciously volunteered. He's on the "To"
>> line to my message here; he's not a member of our dev list.
>>
>> Daniel:
>>
>> Thanks in advance for any assistance / guidance you have to offer.
>>
>> I suggest first reading through this thread:
>>
>> https://lucene.472066.n3.nabble.com/Commit-Code-Review-Policy-td4452928.html
>> I find Nabble much easier to navigate than the ASF official archive, but
>> that's here if you prefer:
>> http://mail-archives.apache.org/mod_mbox/lucene-dev/201911.mbox/%3cCABEwPvET1u4xGxQadddheS+uyBEXMuw33C8fbS5BZDLzULCRag@mail.gmail.com%3e
>> It starts November 26th and ended December 8th.
>>
>> The second thing is my proposal document stored in Confluence. I have
>> yet to rename it as I said I would but I'll let it be for a little bit so
>> the links continue to work for the moment:
>> https://cwiki.apache.org/confluence/display/LUCENE/Commit+Policy+-+DRAFT
>> Everything from "Tests" heading onwards has a "[PENDING DISCUSSION]"
>> notice and can be ignored for the time being.
>>
>> ~ David
>>
>
Re: Commit / Code Review Policy
Posted by Daniel Ruggeri <dr...@apache.org>.
Hi, all;
I could have *sworn* I replied... but could find no record in my sent folder when David dutifully followed up with a ping. *facepalm*
Yes, I volunteered to participate and help where I can as a member of The ASF board, VP Fundraising, ComDev participant, or whatever hat I might be able to bring with me. Of course, I bear no special credentials here... we all hang out as volunteers. Reviewing the most recent board report[1], I detected:
> We're discussing how to semi-require code reviews by defining project guidelines. We invite a board member to help/advice how to navigate this balance.
So, yeah, here I am :-) My primary interest is to be sure we're approaching the discussion with community as our primary lens.
I definitely reviewed the email thread, replies, and Confluence wiki document. In my interpretation of the discussion, a few things stood out:
- There were some synchronous discussions that occurred off the list, which can lead to accidentally excluding important community voices from the conversation. By bringing the discussion back to the list before attempting to make a decision, the critical levels of transparency are maintained. This is definitely A Good Thing.
- I perceived a lot of constructive feedback on the initial draft which appears (to me) to have brought the draft much closer to a document that represents consensus. Said another way, I don't detect conflict per se (please correct me if I'm wrong)
- I think the feedback (and subsequent incorporation of that feedback into the guidelines) about exception cases like doc fixes and "small" changes is brilliant. On the httpd project, as a parallel example, we have a RTC policy for release branches except for docs - which are CTR. Indeed, a "docs committer" is the same as an "httpd committer"... we don't separate privileges because this social contract has worked well for 2 decades.
- The current content of the document seems reasonable given the current environment. One thing that is unclear to me based on a quick browsing of the docs is what the branching strategy is for the project. What I gather is that the master branch is the release branch and features are added to master or merged in from short-lived feature branches. It'd be helpful to clarify this.
- The proposed document draws additional parallels to things that work for httpd. Often times, we will share patches to the list for feedback before commits (similar to the Jira tickets proposed). We also operate trunk as CTR, but require 3 +1 backport votes to the long-lived release branches. This proposal has a similar net effect: bits that get released are (generally) intended to be "actively" reviewed before commit. It does fall short of requiring a consensus-style vote for release branches as httpd does... but that's OK.
All told, this proposal seems quite reasonable to me because it has been discussed by the community, feedback has been incorporated, and the content of the proposal seems totally doable. That said, I'm open to other inquiries if there's something an external perspective can provide. How can I help?
[1] https://whimsy.apache.org/board/minutes/Lucene.html
P.S.
Nabble, mbox, etc... they're OK, but have you tried Ponymail?
https://lists.apache.org/x/thread.html/770ef83a8ed3a3d5d161c4a2cd812b4bdde47464d439c09ec31164dd@%3Cdev.lucene.apache.org%3E
I'm a convert, for sure!
--
Daniel Ruggeri
Director, VP Fundraising, member, httpd PMC
The Apache Software Foundation
On February 7, 2020 9:14:37 AM CST, David Smiley <da...@gmail.com> wrote:
>I neglected to try and close up this long thread on the subject of code
>review guidance. In the project's board report to the ASF, I asked for
>help; Daniel Ruggeri (an ASF VP) graciously volunteered. He's on the
>"To"
>line to my message here; he's not a member of our dev list.
>
>Daniel:
>
>Thanks in advance for any assistance / guidance you have to offer.
>
>I suggest first reading through this thread:
>https://lucene.472066.n3.nabble.com/Commit-Code-Review-Policy-td4452928.html
>I find Nabble much easier to navigate than the ASF official archive,
>but
>that's here if you prefer:
>http://mail-archives.apache.org/mod_mbox/lucene-dev/201911.mbox/%3cCABEwPvET1u4xGxQadddheS+uyBEXMuw33C8fbS5BZDLzULCRag@mail.gmail.com%3e
> It starts November 26th and ended December 8th.
>
>The second thing is my proposal document stored in Confluence. I have
>yet
>to rename it as I said I would but I'll let it be for a little bit so
>the
>links continue to work for the moment:
>https://cwiki.apache.org/confluence/display/LUCENE/Commit+Policy+-+DRAFT
>Everything from "Tests" heading onwards has a "[PENDING DISCUSSION]"
>notice
>and can be ignored for the time being.
>
>~ David
Re: Commit / Code Review Policy
Posted by David Smiley <da...@gmail.com>.
I neglected to try and close up this long thread on the subject of code
review guidance. In the project's board report to the ASF, I asked for
help; Daniel Ruggeri (an ASF VP) graciously volunteered. He's on the "To"
line to my message here; he's not a member of our dev list.
Daniel:
Thanks in advance for any assistance / guidance you have to offer.
I suggest first reading through this thread:
https://lucene.472066.n3.nabble.com/Commit-Code-Review-Policy-td4452928.html
I find Nabble much easier to navigate than the ASF official archive, but
that's here if you prefer:
http://mail-archives.apache.org/mod_mbox/lucene-dev/201911.mbox/%3cCABEwPvET1u4xGxQadddheS+uyBEXMuw33C8fbS5BZDLzULCRag@mail.gmail.com%3e
It starts November 26th and ended December 8th.
The second thing is my proposal document stored in Confluence. I have yet
to rename it as I said I would but I'll let it be for a little bit so the
links continue to work for the moment:
https://cwiki.apache.org/confluence/display/LUCENE/Commit+Policy+-+DRAFT
Everything from "Tests" heading onwards has a "[PENDING DISCUSSION]" notice
and can be ignored for the time being.
~ David
Re: Commit / Code Review Policy
Posted by Noble Paul <no...@gmail.com>.
>“Describe your consideration of what goes in solr-core and what goes in packages or contrib.”
+1 for this
We should first have a clear definition of what's solr-core and how to
create packages
On Fri, Dec 6, 2019 at 3:47 AM Gus Heck <gu...@gmail.com> wrote:
>
> And a link to guidelines on what goes where....
>
> On Thu, Dec 5, 2019 at 10:49 AM Jan Høydahl <ja...@cominvent.com> wrote:
>>
>> The SIP template should have a question that each proposal MUST answer:
>>
>> “Describe your consideration of what goes in solr-core and what goes in packages or contrib.”
>>
>> Jan Høydahl
>>
>> 5. des. 2019 kl. 14:22 skrev Robert Muir <rc...@gmail.com>:
>>
>>
>> Fine, here's my SIP process.
>>
>> You can't add one feature unless you remove two others.
>>
>> Very simple and works.
>>
>> On Thu, Dec 5, 2019 at 4:11 AM Jan Høydahl <ja...@cominvent.com> wrote:
>>>
>>> Let’s keep this thread about code review guidelines, not about feature removal, please. And be concrete with proposals for re-wording Davids proposal instead of sidetracking.
>>> As David said, we need to do both. I think the SIP process for new features and APIs may be a far better way than some hard «feature freeze».
>>>
>>> Jan
>>>
>>> > 4. des. 2019 kl. 22:49 skrev Noble Paul <no...@gmail.com>:
>>> >
>>> >> I don't think this has anything to do with code review: it has to do with people just piling in features, but not taking the time to do any janitorial work or remove old features that shouldn't be there anymore (I AM LOOKING AT YOU HDFS)
>>> >
>>> > 100 %. If there is one problem with Solr today, it is feature bloat.
>>> > We need to trim down Solr by atleast 50%
>>> >
>>> > What we need to do urgently is to create a white list of essential
>>> > features and eliminate others. We are just getting crushed by the
>>> > amount of code we have in Solr. We don't have so many people who can
>>> > actively maintain such a large codebase
>>> >
>>> > On Thu, Dec 5, 2019 at 7:34 AM David Smiley <da...@gmail.com> wrote:
>>> >>
>>> >> 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 <md...@apache.org> 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. 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.
>>> >>
>>> >>
>>> >> 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
>>> >
>>> >
>>> >
>>> > --
>>> > -----------------------------------------------------
>>> > Noble Paul
>>> >
>>> > ---------------------------------------------------------------------
>>> > To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>>> > For additional commands, e-mail: dev-help@lucene.apache.org
>>> >
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>>> For additional commands, e-mail: dev-help@lucene.apache.org
>>>
>
>
> --
> http://www.needhamsoftware.com (work)
> http://www.the111shift.com (play)
--
-----------------------------------------------------
Noble Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: Commit / Code Review Policy
Posted by Gus Heck <gu...@gmail.com>.
And a link to guidelines on what goes where....
On Thu, Dec 5, 2019 at 10:49 AM Jan Høydahl <ja...@cominvent.com> wrote:
> The SIP template should have a question that each proposal MUST answer:
>
> “Describe your consideration of what goes in solr-core and what goes in
> packages or contrib.”
>
> Jan Høydahl
>
> 5. des. 2019 kl. 14:22 skrev Robert Muir <rc...@gmail.com>:
>
>
> Fine, here's my SIP process.
>
> You can't add one feature unless you remove two others.
>
> Very simple and works.
>
> On Thu, Dec 5, 2019 at 4:11 AM Jan Høydahl <ja...@cominvent.com> wrote:
>
>> Let’s keep this thread about code review guidelines, not about feature
>> removal, please. And be concrete with proposals for re-wording Davids
>> proposal instead of sidetracking.
>> As David said, we need to do both. I think the SIP process for new
>> features and APIs may be a far better way than some hard «feature freeze».
>>
>> Jan
>>
>> > 4. des. 2019 kl. 22:49 skrev Noble Paul <no...@gmail.com>:
>> >
>> >> I don't think this has anything to do with code review: it has to do
>> with people just piling in features, but not taking the time to do any
>> janitorial work or remove old features that shouldn't be there anymore (I
>> AM LOOKING AT YOU HDFS)
>> >
>> > 100 %. If there is one problem with Solr today, it is feature bloat.
>> > We need to trim down Solr by atleast 50%
>> >
>> > What we need to do urgently is to create a white list of essential
>> > features and eliminate others. We are just getting crushed by the
>> > amount of code we have in Solr. We don't have so many people who can
>> > actively maintain such a large codebase
>> >
>> > On Thu, Dec 5, 2019 at 7:34 AM David Smiley <da...@gmail.com>
>> wrote:
>> >>
>> >> 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 <md...@apache.org> 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.
>> 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.
>> >>
>> >>
>> >> 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
>> >
>> >
>> >
>> > --
>> > -----------------------------------------------------
>> > Noble Paul
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>> > For additional commands, e-mail: dev-help@lucene.apache.org
>> >
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>> For additional commands, e-mail: dev-help@lucene.apache.org
>>
>>
--
http://www.needhamsoftware.com (work)
http://www.the111shift.com (play)
Re: Commit / Code Review Policy
Posted by Jan Høydahl <ja...@cominvent.com>.
The SIP template should have a question that each proposal MUST answer:
“Describe your consideration of what goes in solr-core and what goes in packages or contrib.”
Jan Høydahl
> 5. des. 2019 kl. 14:22 skrev Robert Muir <rc...@gmail.com>:
>
>
> Fine, here's my SIP process.
>
> You can't add one feature unless you remove two others.
>
> Very simple and works.
>
>> On Thu, Dec 5, 2019 at 4:11 AM Jan Høydahl <ja...@cominvent.com> wrote:
>> Let’s keep this thread about code review guidelines, not about feature removal, please. And be concrete with proposals for re-wording Davids proposal instead of sidetracking.
>> As David said, we need to do both. I think the SIP process for new features and APIs may be a far better way than some hard «feature freeze».
>>
>> Jan
>>
>> > 4. des. 2019 kl. 22:49 skrev Noble Paul <no...@gmail.com>:
>> >
>> >> I don't think this has anything to do with code review: it has to do with people just piling in features, but not taking the time to do any janitorial work or remove old features that shouldn't be there anymore (I AM LOOKING AT YOU HDFS)
>> >
>> > 100 %. If there is one problem with Solr today, it is feature bloat.
>> > We need to trim down Solr by atleast 50%
>> >
>> > What we need to do urgently is to create a white list of essential
>> > features and eliminate others. We are just getting crushed by the
>> > amount of code we have in Solr. We don't have so many people who can
>> > actively maintain such a large codebase
>> >
>> > On Thu, Dec 5, 2019 at 7:34 AM David Smiley <da...@gmail.com> wrote:
>> >>
>> >> 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 <md...@apache.org> 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. 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.
>> >>
>> >>
>> >> 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
>> >
>> >
>> >
>> > --
>> > -----------------------------------------------------
>> > Noble Paul
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>> > For additional commands, e-mail: dev-help@lucene.apache.org
>> >
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>> For additional commands, e-mail: dev-help@lucene.apache.org
>>
Re: Commit / Code Review Policy
Posted by Robert Muir <rc...@gmail.com>.
Fine, here's my SIP process.
You can't add one feature unless you remove two others.
Very simple and works.
On Thu, Dec 5, 2019 at 4:11 AM Jan Høydahl <ja...@cominvent.com> wrote:
> Let’s keep this thread about code review guidelines, not about feature
> removal, please. And be concrete with proposals for re-wording Davids
> proposal instead of sidetracking.
> As David said, we need to do both. I think the SIP process for new
> features and APIs may be a far better way than some hard «feature freeze».
>
> Jan
>
> > 4. des. 2019 kl. 22:49 skrev Noble Paul <no...@gmail.com>:
> >
> >> I don't think this has anything to do with code review: it has to do
> with people just piling in features, but not taking the time to do any
> janitorial work or remove old features that shouldn't be there anymore (I
> AM LOOKING AT YOU HDFS)
> >
> > 100 %. If there is one problem with Solr today, it is feature bloat.
> > We need to trim down Solr by atleast 50%
> >
> > What we need to do urgently is to create a white list of essential
> > features and eliminate others. We are just getting crushed by the
> > amount of code we have in Solr. We don't have so many people who can
> > actively maintain such a large codebase
> >
> > On Thu, Dec 5, 2019 at 7:34 AM David Smiley <da...@gmail.com>
> wrote:
> >>
> >> 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 <md...@apache.org> 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.
> 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.
> >>
> >>
> >> 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
> >
> >
> >
> > --
> > -----------------------------------------------------
> > Noble Paul
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> > For additional commands, e-mail: dev-help@lucene.apache.org
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>
>
Re: Commit / Code Review Policy
Posted by Jan Høydahl <ja...@cominvent.com>.
Let’s keep this thread about code review guidelines, not about feature removal, please. And be concrete with proposals for re-wording Davids proposal instead of sidetracking.
As David said, we need to do both. I think the SIP process for new features and APIs may be a far better way than some hard «feature freeze».
Jan
> 4. des. 2019 kl. 22:49 skrev Noble Paul <no...@gmail.com>:
>
>> I don't think this has anything to do with code review: it has to do with people just piling in features, but not taking the time to do any janitorial work or remove old features that shouldn't be there anymore (I AM LOOKING AT YOU HDFS)
>
> 100 %. If there is one problem with Solr today, it is feature bloat.
> We need to trim down Solr by atleast 50%
>
> What we need to do urgently is to create a white list of essential
> features and eliminate others. We are just getting crushed by the
> amount of code we have in Solr. We don't have so many people who can
> actively maintain such a large codebase
>
> On Thu, Dec 5, 2019 at 7:34 AM David Smiley <da...@gmail.com> wrote:
>>
>> 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 <md...@apache.org> 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. 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.
>>
>>
>> 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
>
>
>
> --
> -----------------------------------------------------
> Noble Paul
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: Commit / Code Review Policy
Posted by Noble Paul <no...@gmail.com>.
> I don't think this has anything to do with code review: it has to do with people just piling in features, but not taking the time to do any janitorial work or remove old features that shouldn't be there anymore (I AM LOOKING AT YOU HDFS)
100 %. If there is one problem with Solr today, it is feature bloat.
We need to trim down Solr by atleast 50%
What we need to do urgently is to create a white list of essential
features and eliminate others. We are just getting crushed by the
amount of code we have in Solr. We don't have so many people who can
actively maintain such a large codebase
On Thu, Dec 5, 2019 at 7:34 AM David Smiley <da...@gmail.com> wrote:
>
> 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 <md...@apache.org> 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. 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.
>
>
> 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
--
-----------------------------------------------------
Noble Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: Commit / Code Review Policy
Posted by David Smiley <da...@gmail.com>.
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 <md...@apache.org> 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.
> 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.
>
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
>
Re: Commit / Code Review Policy
Posted by Mike Drob <md...@apache.org>.
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 <da...@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, <rc...@gmail.com> wrote:
>>
>>>
>>>
>>> On Mon, Dec 2, 2019 at 3:33 PM David Smiley <da...@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 :)
>>>
>>>
>>>
Re: Commit / Code Review Policy
Posted by David Smiley <da...@gmail.com>.
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, <rc...@gmail.com> wrote:
>
>>
>>
>> On Mon, Dec 2, 2019 at 3:33 PM David Smiley <da...@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 :)
>>
>>
>>
Re: Commit / Code Review Policy
Posted by Ishan Chattopadhyaya <ic...@gmail.com>.
> 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, <rc...@gmail.com> wrote:
>
>
> On Mon, Dec 2, 2019 at 3:33 PM David Smiley <da...@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 :)
>
>
>
Re: Commit / Code Review Policy
Posted by Robert Muir <rc...@gmail.com>.
On Mon, Dec 2, 2019 at 3:33 PM David Smiley <da...@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 :)
Re: Commit / Code Review Policy
Posted by David Smiley <da...@gmail.com>.
The document I wrote is hereby a commit guideline document, not policy.
I'll retitle at a later point; it'll change the link. I'll rework a bit so
that the tone doesn't sound too absolute.
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...
Gus wrote:
>
> 1. 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
>
> I was thinking of this the other day. Hmm. Lets start a new thread on
_how_ to get reviews; it's an important subject. The document encourages
us to ask each other for reviews. Lets make this a habit. The health of
the codebase is at stake.
Rob then Thömas wrote:
> > 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.
>
Yes, the doc should somehow make reference to and condone lazy-consensus.
It's a last-resort but always an option. In Solr we need to take steps to
get the reviews so that lazy-consensus is rare. For Lucene this is rare, I
think.
Rob wrote:
> Please don't add unnecessary things to this document like "Linear git
> history" which have nothing to do with code reviews.
>
I considered this. I think it's useful to have one document/page related
to the guidelines of committing code with all that it entails. Many of the
items are short and shouldn't get too long (I think). But I totally get
your point that it's too much to discuss/debate at once. I will expressly
mark those parts as "[PENDING DISCUSSION]" so we can focus on the review
aspect now -- the most important topic.
~ David Smiley
Apache Lucene/Solr Search Developer
http://www.linkedin.com/in/davidwsmiley
On Mon, Dec 2, 2019 at 1:18 PM Anshum Gupta <an...@anshumgupta.net> wrote:
> 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 <da...@gmail.com>
> 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
>>
>
>
> --
> Anshum Gupta
>
Re: Commit / Code Review Policy
Posted by Anshum Gupta <an...@anshumgupta.net>.
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 <da...@gmail.com>
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
>
--
Anshum Gupta