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
_______________________________________________
OpenStack-dev mailing list
[email protected]
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
