On 2014-09-10 09:57:17 +0100 (+0100), Steven Hardy wrote: > I can understand this argument, and perhaps an auto-abandon with a > long period like say a month for the submitter to address comments > and reset the clock would be a reasonable compromise?
Perhaps, but the old script lacked a bunch of things we'd need for this (it didn't understand/skip WIP changes, it also abandoned changes with no reviews at all, it didn't support for the version of Gerrit we're running now) and was tied to old cruft we've excised (particularly the Launchpad->Gerrit group membership sync code) so would likely need to be rewritten mostly from scratch. Probably if we were to do it right, it would be something like a new kind of periodic meta-job job in Zuul which could be selectively added to projects who want it. > The problem we have now, is there is a constantly expanding queue > of zombie reviews, where the submitter got some negative feedback > and, for whatever reason, has chosen not to address it. I agree that's a problem, but I'm unconvinced it's because we lack some system to automatically make them go away. > For example, in my "Incoming reviews" queue on the gerrit > dashboard, I've got reviews I (or someone) -1'd over four months > ago, with zero feedback from the submitter, what value is there to > these reviews cluttering up the dashboard for every reviewer? The moment you, as a core reviewer, abandon one of those with a friendly note it will cease to clutter up the dashboard for anyone. But doing so also gives you an opportunity to possibly notice that this is actually a valuable bug fix/enhancement for your project and take it over. If it gets automatically abandoned because some random reviewer did a drive-by -1 about whitespace preferences a month ago, then you may never know. > To make matters worse Jenkins comes along periodically and > rechecks or figures out the patch merge failed and bumps the > zombie back up to the top of the queue - obviously I don't realise > that it's not a human comment I need to read until I've expended > effort clicking on the review and reading it :( If you, again as a core reviewer, notice that there is a change which needs work but perhaps is not in a situation where outright abandonment is warranted, mark it with workflow -1 (work in progress) then that too will break the cycle. Also Zuul is only spontaneously leaving votes on changes which suddenly cease to be able to merge due to merge conflicts appearing in the target branch, and will only do that to a patchset once... it doesn't periodically retest any changes beyond that without some intervention. > From a reviewer perspective, it's impossible, and means the review > dashboard is basically unusable without complex queries to weed > out the active reviews from the zombies. I would argue that the default Gerrit dashboard is effectively unusable for a variety of other reasons anyway, which is why there are custom dashboards proliferating (globally, per-project and per-user) to maximize reviewer efficiency and use cases. For that matter, it's also arguable that's one of the driving factors behind the increasing number of Gerrit clients and front-end replacements. > All we need is two things: Well, three... 0. A clearly documented set of expectations of code contributors so that they know in advance their proposed changes will be discarded if they do not look after them fairly continually to address comments as they come. This should include the expectation that they may have to gratuitously submit new patchsets from time to time to clear drive-by negative reviews which make pointless demands or are based on misunderstandings of those patches (because core reviewers will not be able to clear other reviewer's -1 votes manually and the original reviewer may never revisit that change), or continue to un-abandon their changes while waiting for proper reviews to find them. Also some custom query they check from time to time to spot their auto-abandoned reviews so they can restore them (this was one of my standard searches back when we used the auto-abandoner). > 1. A way to automatically escalate reviews which have no feedback > at all from core reviewers within a reasonable time (say a week or > two) As a view in reviewday? Or using something like next-review (which I believe has heuristics for selecting neglected reviews for you)? > 2. A way to automatically remove reviews from the queue which have > core negative feedback with no resolution within a reasonable time > (say a month or several weeks, so it's not percieved > contributor-hostile). Well, here's part of the challenge... "core reviewer" is merely a concept we invented. We have Gerrit ACLs which allow anyone to leave a -1 review and also allow *some* reviewers based on a somewhat flexible group membership system to leave -2 reviews. Gerrit itself does not see a -1 review from a so-called "core" reviewer as being any different from one left by any other reviewer and provides no general means to query based on that distinction. Any system which treats some -1 reviews differently from others will require a non-trivial implementation--it would probably need to parse Gerrit's ACL configuration files to identify the correct groups and then iterate over API calls to build recursive maps of group membership (keeping in mind that Gerrit's groups can include other groups to an arbitrary depth). I also think treating some -1 reviews differently from others, while perhaps somewhat unavoidable in practice, is not something we should encode and reinforce because it tears at the egalitarian and populist nature of our current review system. Core reviewers are fundamentally shepherds of other reviewers and gatekeepers of the source code repositories, but a legitimate concern raised by a reviewer should be taken seriously regardless of whether they're a shepherd or a carpenter. > Note (2) still allows "core reviewers to decide if a change has > potential", it just becomes opt-in, e.g we have to address the > issues which prevent us giving it positive feedback, either by > directly working with the contributor, or delegating the work to > an active contributor if the original patch author has decided not > to continue work on the patch. That is unless they don't see them in time. According to Russell's review stats, Infra merged 11.1 changes a day in the past month with a 6-person core team each doing an average of 6.7 reviews a day, yet our backlog still grew by 3.1 changes a day. Needless to say we have some changes which, due to need for scheduling/coordination or just sheer lack of reviewer bandwidth sit for months. Back when we used the auto-abandoner, I had a mail filter to a separate inbox I used to monitor what changes expired so I could manually restore them. I spent far more time restoring changes which shouldn't have been abandoned than I now spend abandoning changes which should. Perhaps my experience is an aberration rather than the norm, but I've broadened the subject tag and revised the topic of this subthread to collect more feedback on whether this is something of interest to projects beyond those in the Deployment program or simply an anti-pattern. > Ultimately, it's not just about reviews - who's going to maintain > all these features if the author is not committed enough to post > just one patch a month while getting it through the review > process? Oh wait, that'll be another job for the core team won't > it? Not all changes bring new features (I certainly hope many of them, if not the majority, fix bugs instead). I'm in the "polish what we have, no new features for a while" camp myself, but... um... you've just proposed a new infrastructure feature. I'll assume from your comment that you don't expect the Infra core team to maintain it. -- Jeremy Stanley _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev