On Fri, Jul 8, 2016 at 11:40 AM, Atin Mukherjee <amukherj@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> To: "Gluster Devel" <gluster-devel@xxxxxxxxxxx>
> Sent: Friday, July 8, 2016 2:02:27 AM
> Subject: 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@xxxxxxxxxxx
> http://www.gluster.org/mailman/listinfo/gluster-devel
>
_______________________________________________
Gluster-devel mailing list
Gluster-devel@xxxxxxxxxxx
http://www.gluster.org/mailman/listinfo/gluster-devel
_______________________________________________
Gluster-devel mailing list
Gluster-devel@xxxxxxxxxxx
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
_______________________________________________ Gluster-devel mailing list Gluster-devel@xxxxxxxxxxx http://www.gluster.org/mailman/listinfo/gluster-devel
