<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 8, 2016 at 11:23 AM, Poornima Gurusiddaiah <span dir="ltr"><<a href="mailto:pgurusid@redhat.com" target="_blank">pgurusid@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Completely agree with your concern here. Keeping aside the regression part, few observations and suggestions:<br>
As per the Maintainers guidelines (<a href="https://gluster.readthedocs.io/en/latest/Contributors-Guide/Guidelines-For-Maintainers/" rel="noreferrer" target="_blank">https://gluster.readthedocs.io/en/latest/Contributors-Guide/Guidelines-For-Maintainers/</a>):<br>
<br>
a> Merge patches of owned components only.<br>
b> Seek approvals from all maintainers before merging a patchset spanning multiple components.<br>
c> Ensure that regression tests pass for all patches before merging.<br>
d> Ensure that regression tests accompany all patch submissions.<br>
e> Ensure that documentation is updated for a noticeable change in user perceivable behavior or design.<br>
f> Encourage code unit tests from patch submitters to improve the overall quality of the codebase.<br>
g> Not merge patches written by themselves until there is a +2 Code Review vote by other reviewers.<br>
<br>
Clearly a, b, are not being strictly followed, because of multiple reasons.<br>
- Not every component in Gluster has a Maintainer<br></blockquote><div><br>We need to fix this. Do you want to take up the task of coming up with a list?<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- Its getting difficult to get review time from maintainers as they are maintainers for several component, and they are also active developers.<br></blockquote><div><br>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.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- What is enforced by mere documentation of procedure, is hard to implement.<br>
<br>
Below are the few things that we can do to reduce our review backlog:<br>
- 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? </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- Add maintainers for every component that is in Gluster(atleast the ones which have incoming patches)<br>
- 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.<br>
- 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).<br>
The release manager should ensure that there are no patches in below gerrit search link provided by Jeff.<br>
<br>
Any thoughts?<br></blockquote><div><br></div><div>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. <br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Regards,<br>
Poornima<br>
<div class=""><div class="h5"><br>
----- Original Message -----<br>
> From: "Jeff Darcy" <<a href="mailto:jdarcy@redhat.com">jdarcy@redhat.com</a>><br>
> To: "Gluster Devel" <<a href="mailto:gluster-devel@gluster.org">gluster-devel@gluster.org</a>><br>
> Sent: Friday, July 8, 2016 2:02:27 AM<br>
> Subject: [Gluster-devel] Reducing merge conflicts<br>
><br>
> I'm sure a lot of you are pretty frustrated with how long it can take to get<br>
> even a trivial patch through our Gerrit/Jenkins pipeline. I know I am.<br>
> Slow tests, spurious failures, and bikeshedding over style issues are all<br>
> contributing factors. I'm not here to talk about those today. What I am<br>
> here to talk about is the difficulty of getting somebody - anybody - to look<br>
> at a patch and (possibly) give it the votes it needs to be merged. To put<br>
> it bluntly, laziness here is *killing* us. The more patches we have in<br>
> flight, the more merge conflicts and rebases we have to endure for each one.<br>
> It's a quadratic effect. That's why I personally have been trying really<br>
> hard to get patches that have passed all regression tests and haven't gotten<br>
> any other review attention "across the finish line" so they can be merged<br>
> and removed from conflict with every other patch still in flight. The<br>
> search I use for this, every day, is as follows:<br>
><br>
> <a href="http://review.gluster.org/#/q/status:open+project:glusterfs+branch:master+label:CentOS-regression%253E0+label:NetBSD-regression%253E0+-label:Code-Review%253C0" rel="noreferrer" target="_blank">http://review.gluster.org/#/q/status:open+project:glusterfs+branch:master+label:CentOS-regression%253E0+label:NetBSD-regression%253E0+-label:Code-Review%253C0</a><br>
><br>
> That is:<br>
><br>
> open patches on glusterfs master (change project/branch as appropriate to<br>
> your role)<br>
><br>
> CentOS and NetBSD regression tests complete<br>
><br>
> no -1 or -2 votes which might represent legitimate cause for delay<br>
><br>
> If other people - especially team leads and release managers - could make a<br>
> similar habit of checking the queue and helping to get such "low hanging<br>
> fruit" out of the way, we might see an appreciable increase in our overall<br>
> pace of development. If not, we might have to start talking about mandatory<br>
> reviews with deadlines and penalties for non-compliance. I'm sure nobody<br>
> wants to see their own patches blocked and their own deadlines missed<br>
> because they weren't doing their part to review peers' work, but that's a<br>
> distinct possibility. Let's all try to get this train unstuck and back on<br>
> track before extreme measures become necessary.<br>
> _______________________________________________<br>
> Gluster-devel mailing list<br>
> <a href="mailto:Gluster-devel@gluster.org">Gluster-devel@gluster.org</a><br>
> <a href="http://www.gluster.org/mailman/listinfo/gluster-devel" rel="noreferrer" target="_blank">http://www.gluster.org/mailman/listinfo/gluster-devel</a><br>
><br>
</div></div></blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr">Pranith<br></div></div>
</div></div>