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