Hi Alex,
Interesting question. A few initial thoughts:
*YOML*
YOML (PR #363) is an exceptional case - we should not use that as an
example when discussing this meta-question. The PR is 12,000 lines
(including comments/notes), and was not discussed on the mailing list
before it was submitted. I suggest we have a separate email thread
specifically about merging that PR, as there are certainly very useful
things we'd get from YOML.
*Small PRs*
We should strongly encourage small focused PRs on a single thing,
wherever possible. That will make review faster, easier and lower risk.
For such PRs, we should strive for review+merge within days (7 days
being an upper bound in normal circumstances, hopefully).
We can add some brief guidelines to this effect at
http://brooklyn.apache.org/developers/how-to-contribute.html
*Changing low-level Brooklyn*
PRs that change low-level things in Brooklyn (e.g. changes to
config-inheritance etc) deserve thorough review. They are high-risk as
the unforeseen consequences of the changes can be very subtle, and break
downstream blueprints that rely on old ways of doing things.
*Light-weight Review*
I agree with you - where PRs look sensible, low-risk and unit tested we
should take more risk and merge them sooner (even if there's not been
time for a thorough review by the community).
Aled
On 02/05/2017 15:50, Duncan Johnston Watt wrote:
Hi Alex
This is probably covered already but I guess there needs to be an impact
assessment (by submitter?) before something is waved through by default.
Best
Duncan
On 2 May 2017 at 06:52, Alex Heneveld <[email protected]>
wrote:
Hi Brooklyners-
As many of you know, my YOML PR #363 [1] has been open for a while. This
sets up a foundation for giving better documentation and feedback and
hugely simplifying how we do our parsing. However it's a very big PR. I'm
eager to have people spend some time using it and ideally extending it --
but here I wanted to raise a meta-question:
*W**hat should we do with a PR when we aren't able to review things in as
much depth as we'd like?*
One option -- call it (A) -- is to say if we can't review things
thoroughly in a reasonable timeframe, we do a lighter review and if the PR
looks promising and safe we merge it.
The other option -- call it (B) -- is to leave PRs open for as long as it
takes for us to do the complete review.
I think most people have been approaching this with a mindset of (B), and
while that's great for code quality and shared code understanding, if we
can't deliver on that quickly, it's frankly anti-social. The contributor
has to deal with merge conflicts (and the rudeness of his or her
contribution being ignored), and Brooklyn loses velocity. My PR is an
extreme example but many have been affected by slow reviews, and I think
the expectation that reviews have to be so thorough is part of the
problem: it even discourages reviewers, as if you're not an expert in an
area you probably don't feel qualified to review.
We have good test coverage so product risk of (A) is small, and we have
great coders so I've no worry about us being able to solve problems that
(A) might introduce. We should be encouraging reviewers to look at any
area, and we need to solve the problem of slow reviews.
*I propose that the**standard we apply is that we quickly either merge PRs
or identify what the contributor needs to resolve.
*I'm all for thorough reviews and shared understanding, but if we can't do
this quickly I suggest we are better to sacrifice those things rather than
block contributions, stifle innovation, and discourage reviews by insisting
on a standards that we struggle to sustain.
As a general rule of thumb, maybe something like:
(1) After 7 days of no activity on a PR we go with an "eyeball test";
unless the following statement is untrue we say:
/I haven't done as much review as I'd like, but the code is clearly
helpful, not risky or obviously wrong or breaking compatibility, it has
good test coverage, and we can reasonably expect the contributor or
committers to maintain it. Leaving open a bit longer in case someone else
wants to review more but if nothing further in the next few days, let's
merge.
/(If there are committers who are likely to be specifically interested,
call them out as CC.)
(2) After 3 more days, if no activity, merge it.
And we encourage _anyone_ to review anything. If the above response is
the baseline, everyone in our community is qualified to do it or better and
we'll be grateful!
Best
Alex
[1] https://github.com/apache/brooklyn-server/pull/363
