On Fri, Jul 8, 2016 at 11:40 AM, Atin Mukherjee <amukh...@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 < > pguru...@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" <jda...@redhat.com> >> > To: "Gluster Devel" <gluster-devel@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@gluster.org >> > http://www.gluster.org/mailman/listinfo/gluster-devel >> > >> _______________________________________________ >> Gluster-devel mailing list >> Gluster-devel@gluster.org >> http://www.gluster.org/mailman/listinfo/gluster-devel >> > > > _______________________________________________ > Gluster-devel mailing list > Gluster-devel@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
_______________________________________________ Gluster-devel mailing list Gluster-devel@gluster.org http://www.gluster.org/mailman/listinfo/gluster-devel