Open Stack

On 07/11/2014 06:30 PM, Kevin L. Mitchell wrote:
> I decided to do some Nova reviews today, and I decided to do it by
> pulling up the list of all of them and start from the end.  What I've
> found is a *lot* of reviews that have been idle for several months.
> I've even found one or two that were approved, but weren't merged due to
> depending on an outdated patch, and I found others that had several +1s
> but no -1s or +2s.

Hi Vek :)

I, too, tend to see a lot of such patches, as I try to review things 
that don't necessarily have a bunch of feedback already on them.

Here are my general rules of thumb and/or ideas about these classes of 
patches:

1) If the patch was approved but failed to merge because of an abandoned 
or outdated dependent patchset, then I contact the patch submitter via 
email and just say "Hey, do you need help getting this patch going 
again? If not, please update the dependent patch or abandon the patch."

2) If the patch has 2 +2s from core reviewers but is not +W'd, then I 
will find a core on #openstack-nova and ask them to do a quick re-review 
and +W the patch if all looks good.

3) If the patch has a couple +1s but no -1 or +/-2s, then I actually get 
interested in the patch and will do a thorough review of it. I will 
leave my personal review, and do one of the following:

  a) If I think the patch is an important fix or feature piece 
(regardless of whether I think the patch is good or not), I will ping a 
core in #openstack-nova to get core opinion on the patch
  b) If I think the patch is NOT a particularly important fix or feature 
piece, I might ask some non-core reviewers to take a look and evaluate 
it. At least if the non-core reviewers review it (good or bad), then the 
patch will bump to the head of many patch queues and may get some extra 
eyeballs on it that way.

> There are 2 or 3 pages of these old reviews hanging
> at the end of the list, and it made me ask about the auto-expiration we
> used to have—I missed it in the Gerrit upgrade thread, but it turns out
> that, since core reviewers can now abandon/restore patches, the
> auto-expiration has been turned off.
>
> Given that we have so many old reviews hanging around on nova (and
> probably other projects), should we consider setting something like that
> back up?  With nova, at least, the vast majority of them can't possibly
> merge because they're so old, so we need to at least have something to
> remind the developer that they need to rebase…and if they've forgotten
> the review or don't care about it anymore, we should either have it
> taken over or get the review abandoned.

I didn't like the impersonal nature of the auto-expire thing, frankly. I 
prefer the current situation where deliberate action is needed, even if 
that means a little more work for the core review team.

> The other concern I have is the several reviews that no core dev looked
> at in an entire month, but I have no solutions to suggest there,
> unfortunately :(

Patches of your own or patches of other folks?

All the best,
-jay