[Gluster-devel] Reducing merge conflicts

Fri Jul 8 07:34:23 UTC 2016

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
>

We need to fix this. Do you want to take up the task of coming up with a
list?

> - Its getting difficult to get review time from maintainers as they are
> maintainers for several component, and they are also active developers.
>

Is it your experience that the patch is not at all getting a single review
or there are no other people who can review? In my experience even when
there were other people who could do the reviews people want to lean on
maintainers to do the initial reviews because they would find most of the
problems in the first review. I am guilty of leaning on the main
maintainers too :-(. If this happens others in the team won't improve in
finding issues in reviewing others'/their own patches. Did you guys already
solve this problem in the components you are working on? What are you guys
doing for improving in reviews/get more participation? In our team both
Krutika and Ravi frequent top-10 people who send patches per month, so it
was too much for 1 maintainer to take this kind of load. Everyone in the
team started reviewing the patches and giving +1 and I am reviewing only
after a +1. It still feels a bit skewed though.

- 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 automaticallyIn our team both Krutika and Ravi frequent top-10
> people who send patches per month, so it was too much for 1 maintainer to
> take this kind of load. Everyone in the team started reviewing the patches
> and giving +1 and I am reviewing only after a +1. My hope is this will lead
> to faster patch acceptance over time. add maintainers as reviewers, and
> have another label 'Maintainers ack' which needs to be present for any
> patch to be merged.
> - 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).
>   The release manager should ensure that there are no patches in below
> gerrit search link provided by Jeff.
>
> Any thoughts?
>

I am in favour of more people knowing more components in the
stack(preferably more projects). What I have seen from my experience is
that you would be able to come up with solutions fast because you would
have seen the problem solved in different ways in these different
components/projects. Reviewing is one way to gain more knowledge about a
different component. Ashish surprises me with his reviews sometimes even
when he doesn't know much about the component he is reviewing. So how can
we encourage more people to pick up new components? Do you have any ideas?
Getting more reviews will be a very small problem if we have more
knowledgeable people per component.

> 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
> >
>

-- 
Pranith
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.gluster.org/pipermail/gluster-devel/attachments/20160708/1e3fbda5/attachment.html>