lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Noble Paul <>
Subject Re: Commit / Code Review Policy
Date Mon, 09 Dec 2019 00:27:44 GMT
>“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 <> wrote:
> And a link to guidelines on what goes where....
> On Thu, Dec 5, 2019 at 10:49 AM Jan Høydahl <> 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 <>:
>> 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 <> 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 <>:
>>> >
>>> >> 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 <>
>>> >>
>>> >> Mike D.,
>>> >>  I loved your response, especially for researching what other projects
>>> >>
>>> >> ... more responses within...
>>> >>
>>> >> On Tue, Dec 3, 2019 at 2:42 PM Mike Drob <> 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.
>>> >>> HBase requires a review and has an exception for trivial patches
>>> >>> Yetus requires reviews, but allows binding review from non-committers,
and has a no review expiration. - 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. -
>>> >>>
>>> >>> 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:
>>> > For additional commands, e-mail:
>>> >
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail:
>>> For additional commands, e-mail:
> --
> (work)
> (play)

Noble Paul

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message