[openstack-dev] [trove] Request for transparent Gerrit approval process
Morgan Jones
morgan at parelastic.com
Tue May 6 14:17:44 UTC 2014
I've been thinking about this a bit, and had some ideas. Take the
following as points for thought, rather than my saying "this is what we
should do".
How about each Trove participant be assigned a core mentor. This way,
each non-core person knows who they can ask for information about how
OpenStack and Trove work.
During the blueprint review process, part of "approving" a blueprint
would be assigning a core member to oversee the implementation of that
blueprint. The assigned core member needn't have an active role in the
implementation of the blueprint, but they would be available to answer
questions and provide assistance where needed. Likewise, part of
triaging a bug would be assigning a core member to oversee it.
When a blueprint or bug is complete and ready for review, it would be
reviewed by the implementer's mentor and the core member who is
overseeing the bp/bug. This way, reviews needn't sit around waiting for
someone to notice them.
Hopefully, such a system would help core members plan their workload.
It should also make non-core member's integration into the team faster
and easier, and perhaps even speed increasing the core team by
providing consistent mentoring.
Thoughts?
Morgan.
On 05/05/2014 10:29 PM, Nikhil Manchanda wrote:
> 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
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
More information about the OpenStack-dev
mailing list