[Gluster-devel] Reducing merge conflicts

[Atin Mukherjee]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.gluster.org/pipermail/gluster-devel/attachments/20160708/4cc440cb/attachment.html>