Open Stack

Hi Mat:

Some answers, and my perspective, are inline:

Lowery, Mathew writes:

> As a non-core, I would like to understand the process by which core
> prioritizes Gerrit changes.

I'm not aware of any standard process used by all core reviewers to
prioritize reviewing changes in Gerrit. My process, specifically,
involves looking through trove changes that are in-flight in gerrit, and
picking ones based on priority of the bug / bp fixed, whether or not the
patch is still work-in-progress and age.


> I'd also like to know any specific
> criteria used for approval. If such a process was transparent and
> followed consistently, wouldn't that eliminate the need for "Hey core,
> can you review <change>?" in IRC?

I'm not aware of any specific criteria that we use for approval other
than the more general cross-OpenStack criteria (hacking, PEP8,
etc). Frankly, any rules that constitute a common criteria should really
be enforced in the tox runs (for eg. through hacking). Reviewers should
review patches to primarily ensure that changes make sense in context,
and are sound design-wise -- something that automated tools can't do (at
least not yet).


> Specifically:
>
>   *   Is a prioritized queue used such as the one found at
>   http://status.openstack.org/reviews/? (This output comes from a
>   project called
>   ReviewDay<https://github.com/openstack-infra/reviewday> and it is
>   prioritized based on the Launchpad ticket and age:
>   http://git.io/h3QULg.) If not, how do you keep a Gerrit change from
>   "starving?"

How reviewers prioritize what they want to review is something that is
currently left up to the reviewers. Personally, when I review changes,
I quickly look through all patches on review.o.o and do a quick triage
to decide on the review order. I do look at review-age as part of the
triage, and hopefully this prevents 'starvation' to some extent.

That said, this area definitely seems like one where having a
streamlined, team-wide process can help with getting more bang for our
review-buck. Using something like reviewday could definitely help here.

I'm also curious how some of the other OpenStack teams solve this issue,
and what we can do to align in this regard.


>   *   Is there a baking period? In other words, is there a minimum
>   amount of time that has to elapse before even being considered for
>   approval?

There is no such baking period.


>   *   What effect do -1 code reviews have on the chances of approval
>   (or even looking at the change)? Sometimes, -1 code reviews seem to
>   be given in a cavalier manner. In my mind, -1 code reviewers have a
>   duty to respond to follow-up questions by the author. Changes not
>   tainted with a -1 simply have a greater chance of getting looked at.

I always look at the comment that the -1 review has along with it. If
the concern is valid, and the patch author has not addressed it with a
reply, I will leave a comment (with a -1 in some cases) myself. If the
original -1 comment is not applicable / incorrect, I will +1 / +2 as
applicable, and usually, also leave a comment.


>   *   To harp on this again: Isn't "Hey core, can you review
>   <change>?" inherently unfair assuming that there is a process by
>   which a Gerrit change would normally be prioritized and reviewed in
>   an orderly fashion?

I don't necessarily see this as unfair. When someone asks me to review a
change, I don't usually do it immediately (unless it's a fix for a
blocking / breaking change). I tell them that I'll get to it soon, and
ensure that it's in the list of reviews that I triage and review as
usual.


>   *   Are there specific actions that non-cores can take to assist in
>   the orderly and timely approval of Gerrit changes? (e.g. don't give
>   a -1 code review on a multiple +1'ed Gerrit change when it's a
>   nice-to-have and don't leave a -1 and then vanish)?

IMHO purely 'nice-to-have' nitpicks should be +0, regardless of how
many +1s the review already has. And if someone leaves a -1 on a review
and subsequently refuses to discuss it, that is just bad form on
the part of the reviewer. Thankfully, this hasn't been a big problem
that we've had to contend with.


>
> Any clarification of the process would be greatly appreciated.
>
> Thanks,
> Mat
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev