Open Stack

On Wed, Sep 10, 2014 at 04:37:14PM +0000, Jeremy Stanley wrote:
> 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.

Ok, so lets collect requirements for a smarter auto-abandon script, and
figure out how feasable it is.

> > 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.

It is though, because the current system puts a never-ending increasing
load on a finite and hard-to-grow resource (core reviewers).

Clearly documented auto-abandon puts the load on the never-ending
increasing group of new contributors.

> > 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.

I think the issue is, in practice, this is not happening, for the following
reasons:

- There's no "friendly" way, as a core reviewer, of saying to a contributor
  that their patch is abandoned.  I've never abandoned someone elses patch,
  because I see it as carrying the same message as -2 reviews - it's a
  strongly negative message to send to a contributor.

- As a core reviewer, typically I have zero visibility of whether a
  contributor is actually working on the patch, so what's a reasonable time
  to wait before "taking over" or abandoning a patch?  Not all folks are
  active on IRC and even if they are, it's more -core cycles spent tracking
  folks down and chasing them (often repeatedly) for status updates.

A clearly communicated patch-expiry system solves both of these issues.

> > 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.

Maybe the auto-WIP idea mentioned in the original thread is the way forward
then, as again I don't see -core review cycles doing WIP-bot duty as
particularly worthwhile personally.

> > 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).

Well, yes, but if you have both things I mentioned, you can avoid the
scenario where gratuiitous or wrong non-core negative feedback auto-expires
a patch, because you have an escalation path whereby we're ensuring that
-core attention reaches all patches before expiry by highlighting those
without any core feedback at all.

> > 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)?

Yeah, I don't know what the optimal solution is - my attention has recently
been drawn to queries generated via gerrit-dash-creator, which I'm finding
help a lot.

> > 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.

Restricting an auto-abandon/expire to only expire on negative feedback from
-core group members doesn't in any way make feedback from non-core
reviewers get treated less seriously, it just maintains the status-quo and
ensures -core folks (who, ultimately do make the decision about whether a
patch gets approved and merged) get a chance to see the patch...

> > 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.

Technical challenges re connecting the two conditionals of "has negative
feedback" and "feedback user in $core_group" aside, only expiring reviews
seen by a core reviewer solves all these issues AFAICS.

> > 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.

Well we're discussing requirements, I've not proposed any patches ;)

But yes, I'm of the opinion that if folks contribute code, particularly new
features, they should be willing to stick around and at least try to deal
with any issues in the stuff they wrote.

So it's setting a pretty low bar if the only thing we actually require is
they stick around to respond, infrequently, to review feedback, else click
"restore change" if/when they decide they do want to respond.

Steve