Open Stack

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