Commit / Code Review Policy

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
39 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

david.w.smiley@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


On Mon, Dec 2, 2019 at 1:18 PM Anshum Gupta <[hidden email]> 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 <[hidden email]> wrote:
Last Wednesday at a Solr committers meeting, there was general agreement in attendance to raise the bar for commit permission to require another's consent, which might not have entailed a review of the code.  I volunteered to draft a proposal.  Other things distracted me but I'm finally thinking of this task now.  *This email is NOT the proposal*. 

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


I'd prefer we have one "Commit Policy" document for Lucene/Solr and only call out Solr specifics when applicable.  This is easier to maintain and is in line with the joint-ness of Lucene TLP.  So I think it should move to the Lucene cwiki.  Granted there is a possibility this kind of content might move into our source control somewhere but that possibility is a subject for another day.

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

~ David Smiley
Apache Lucene/Solr Search Developer


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

Re: Commit / Code Review Policy

Robert Muir


On Mon, Dec 2, 2019 at 3:33 PM David Smiley <[hidden email]> 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 :)


Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Ishan Chattopadhyaya
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, <[hidden email]> wrote:


On Mon, Dec 2, 2019 at 3:33 PM David Smiley <[hidden email]> 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 :)


Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

david.w.smiley@gmail.com
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


On Mon, Dec 2, 2019 at 10:15 PM Ishan Chattopadhyaya <[hidden email]> 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, <[hidden email]> wrote:


On Mon, Dec 2, 2019 at 3:33 PM David Smiley <[hidden email]> 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 :)


Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Mike Drob-3
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 <[hidden email]> wrote:
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


On Mon, Dec 2, 2019 at 10:15 PM Ishan Chattopadhyaya <[hidden email]> 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, <[hidden email]> wrote:


On Mon, Dec 2, 2019 at 3:33 PM David Smiley <[hidden email]> 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 :)


Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Doug Turnbull
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 <[hidden email]> 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 <[hidden email]> wrote:
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


On Mon, Dec 2, 2019 at 10:15 PM Ishan Chattopadhyaya <[hidden email]> 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, <[hidden email]> wrote:


On Mon, Dec 2, 2019 at 3:33 PM David Smiley <[hidden email]> 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, LLC | 240.476.9983 
Author: Relevant Search
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.
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Robert Muir

On Tue, Dec 3, 2019 at 3:02 PM Doug Turnbull <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

david.w.smiley@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


On Wed, Dec 4, 2019 at 8:12 AM Robert Muir <[hidden email]> wrote:

On Tue, Dec 3, 2019 at 3:02 PM Doug Turnbull <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

david.w.smiley@gmail.com
In reply to this post by Mike Drob-3
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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Noble Paul നോബിള്‍  नोब्ळ्
> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Jan Høydahl / Cominvent
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 <[hidden email]>:
>
>> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>


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

Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Robert Muir
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 <[hidden email]> 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 <[hidden email]>:
>
>> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>


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

Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

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


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 <[hidden email]> 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 <[hidden email]>:
>
>> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>


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

Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

Gus Heck
And a link to guidelines on what goes where....

On Thu, Dec 5, 2019 at 10:49 AM Jan Høydahl <[hidden email]> 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 <[hidden email]>:


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 <[hidden email]> 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 <[hidden email]>:
>
>> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>


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



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

Re: Commit / Code Review Policy

Noble Paul നോബിള്‍  नोब्ळ्
>“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 <[hidden email]> wrote:

>
> And a link to guidelines on what goes where....
>
> On Thu, Dec 5, 2019 at 10:49 AM Jan Høydahl <[hidden email]> 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 <[hidden email]>:
>>
>> 
>> 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 <[hidden email]> 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 <[hidden email]>:
>>> >
>>> >> 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 <[hidden email]> 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 <[hidden email]> 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: [hidden email]
>>> > For additional commands, e-mail: [hidden email]
>>> >
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [hidden email]
>>> For additional commands, e-mail: [hidden email]
>>>
>
>
> --
> http://www.needhamsoftware.com (work)
> http://www.the111shift.com (play)



--
-----------------------------------------------------
Noble Paul

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

Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

david.w.smiley@gmail.com
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:
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@...%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: 
Everything from "Tests" heading onwards has a "[PENDING DISCUSSION]" notice and can be ignored for the time being.

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

Re: Commit / Code Review Policy

Daniel Ruggeri
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 <[hidden email]> 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:
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@...%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: 
Everything from "Tests" heading onwards has a "[PENDING DISCUSSION]" notice and can be ignored for the time being.

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

Re: Commit / Code Review Policy

david.w.smiley@gmail.com
On Wed, Mar 4, 2020 at 8:36 AM Daniel Ruggeri <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Commit / Code Review Policy

david.w.smiley@gmail.com
In reply to this post by Daniel Ruggeri

On Wed, Mar 4, 2020 at 8:36 AM Daniel Ruggeri <[hidden email]> 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 <[hidden email]> 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:
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@...%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: 
Everything from "Tests" heading onwards has a "[PENDING DISCUSSION]" notice and can be ignored for the time being.

~ David
12