[Gluster-devel] Reducing merge conflicts

Fri Jul 8 06:58:01 UTC 2016

On Fri, Jul 8, 2016 at 11:40 AM, Atin Mukherjee <amukherj at redhat.com> wrote:

> How about having "review marathon" once a week by every team? In past this
> has worked well and I don't see any reason why can't we spend 3-4 hours in
> a meeting on weekly basis to review incoming patches on the component that
> the team owns.
>
> On Fri, Jul 8, 2016 at 11:23 AM, Poornima Gurusiddaiah <
> pgurusid at redhat.com> wrote:
>
>>
>> Completely agree with your concern here. Keeping aside the regression
>> part, few observations and suggestions:
>> As per the Maintainers guidelines (
>> https://gluster.readthedocs.io/en/latest/Contributors-Guide/Guidelines-For-Maintainers/
>> ):
>>
>>     a> Merge patches of owned components only.
>>     b> Seek approvals from all maintainers before merging a patchset
>> spanning multiple components.
>>     c> Ensure that regression tests pass for all patches before merging.
>>     d> Ensure that regression tests accompany all patch submissions.
>>     e> Ensure that documentation is updated for a noticeable change in
>> user perceivable behavior or design.
>>     f> Encourage code unit tests from patch submitters to improve the
>> overall quality of the codebase.
>>     g> Not merge patches written by themselves until there is a +2 Code
>> Review vote by other reviewers.
>>
>> Clearly a, b, are not being strictly followed, because of multiple
>> reasons.
>> - Not every component in Gluster has a Maintainer
>> - Its getting difficult to get review time from maintainers as they are
>> maintainers for several component, and they are also active developers.
>> - What is enforced by mere documentation of procedure, is hard to
>> implement.
>>
>> Below are the few things that we can do to reduce our review backlog:
>> - No time for maintainers to review is not a good enough reason to bitrot
>> patches in review for months, it clearly means we need additional
>> maintainers for that component?
>> - Add maintainers for every component that is in Gluster(atleast the ones
>> which have incoming patches)
>> - For every patch we submit we add 'component(s)' label, and evaluate if
>> gerrit can automatically add maintainers as reviewers, and have another
>> label 'Maintainers ack' which needs to be present for any patch to be
>> merged.
>>
>
> I believe this is something which Nigel is already working on.
>
>
>> - Before every major(or minor also?) release, any patch that is not
>> making to the release should have a '-1' by the maintainer or the developer
>> themselves stating the reason(preferably not no time to review).
>>
>
> IMO, it should be the other way around, if the fix/RFE is a must for the
> upcoming release, it should be attached to the tracker bug to ensure
> release is blocked with out the patch. Having a -1 just for not targeting
> it for a specific release doesn't make sense to me.
>
>
>>   The release manager should ensure that there are no patches in below
>> gerrit search link provided by Jeff.
>>
>
>> Any thoughts?
>>
>> Regards,
>> Poornima
>>
>> ----- Original Message -----
>> > From: "Jeff Darcy" <jdarcy at redhat.com>
>> > To: "Gluster Devel" <gluster-devel at gluster.org>
>> > Sent: Friday, July 8, 2016 2:02:27 AM
>> > Subject: [Gluster-devel] Reducing merge conflicts
>> >
>> > I'm sure a lot of you are pretty frustrated with how long it can take
>> to get
>> > even a trivial patch through our Gerrit/Jenkins pipeline.  I know I am.
>> > Slow tests, spurious failures, and bikeshedding over style issues are
>> all
>> > contributing factors.  I'm not here to talk about those today.  What I
>> am
>> > here to talk about is the difficulty of getting somebody - anybody - to
>> look
>> > at a patch and (possibly) give it the votes it needs to be merged.  To
>> put
>> > it bluntly, laziness here is *killing* us.  The more patches we have in
>> > flight, the more merge conflicts and rebases we have to endure for each
>> one.
>> > It's a quadratic effect.  That's why I personally have been trying
>> really
>> > hard to get patches that have passed all regression tests and haven't
>> gotten
>> > any other review attention "across the finish line" so they can be
>> merged
>> > and removed from conflict with every other patch still in flight.  The
>> > search I use for this, every day, is as follows:
>> >
>> >
>> http://review.gluster.org/#/q/status:open+project:glusterfs+branch:master+label:CentOS-regression%253E0+label:NetBSD-regression%253E0+-label:Code-Review%253C0
>> >
>> > That is:
>> >
>> >     open patches on glusterfs master (change project/branch as
>> appropriate to
>> >     your role)
>> >
>> >     CentOS and NetBSD regression tests complete
>> >
>> >     no -1 or -2 votes which might represent legitimate cause for delay
>> >
>> > If other people - especially team leads and release managers - could
>> make a
>> > similar habit of checking the queue and helping to get such "low hanging
>> > fruit" out of the way, we might see an appreciable increase in our
>> overall
>> > pace of development.  If not, we might have to start talking about
>> mandatory
>> > reviews with deadlines and penalties for non-compliance.  I'm sure
>> nobody
>> > wants to see their own patches blocked and their own deadlines missed
>> > because they weren't doing their part to review peers' work, but that's
>> a
>> > distinct possibility.  Let's all try to get this train unstuck and back
>> on
>> > track before extreme measures become necessary.
>> > _______________________________________________
>> > Gluster-devel mailing list
>> > Gluster-devel at gluster.org
>> > http://www.gluster.org/mailman/listinfo/gluster-devel
>> >
>> _______________________________________________
>> Gluster-devel mailing list
>> Gluster-devel at gluster.org
>> http://www.gluster.org/mailman/listinfo/gluster-devel
>>
>
>
> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel at gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel
>

* I'm working on making sure maintainers need to give an ack.
* We should be very careful about using numbers to motivate things.
* I'm getting a dashboard URL so that the page that the reviews listing is
more easily visible.

-- 
nigelb
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.gluster.org/pipermail/gluster-devel/attachments/20160708/7a1b0b92/attachment-0001.html>