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