Open Stack

On 21/07/15 04:21, Dmitry Borodaenko wrote:
>> I -2 for this reason: this patch already had a +2 so it was closed to be
>> merged by someone else, while the patch was *breaking backward
>> compatiblity* in puppet-horizon, because Vasyl was changing the default
>> cache driver to force users to use Memcached by default.
>> We *never* break backward compatibility (this is a general rule in
>> OpenStack) and *never* change OpenStack default configuration (general
>> rule in Configuration management), but rather provide the interface
>> (called Puppet parameters) to change it.
>
> That's fair enough, and on July 8 we pushed patch set 10 that has fixed
> this problem, and set the default to LocMemCache. AFACT at this point
> you've lost track of this commit because your gerrit dashboard excludes
> all commits that have a -2 vote on them, so it got stuck in limbo for
> two weeks.
>
> I still disagree with your use of -2 vote here, a -1 from a core
> reviewer should be enough to prevent other cores from merging a patch
> set, and should tell them what to watch out for in subsequent patch sets.

This is actually a wider problem across OpenStack - core reviewers in 
different projects have different ideas about what -2 means.

For me, it's a signal to the submitter that there is no way the patch 
could ever be merged in its current form and that they should not bother 
to invest any further effort in it unless there is a radical change in 
approach. That's certainly the way it is used in heat-core. (There's 
also a procedural version, where it's just that the patch certainly 
won't be merged before an imminent feature freeze, but the -2 will be 
removed after that.) It's also the case that most tooling is set up for 
(e.g. -2'd patches drop off everyone's review radar).

At the other end of the spectrum, I've heard that core reviewers in some 
projects (*cough*nova*cough*) will routinely -2 a patch without having 
even read it just to be sure that it cannot get merged without their 
personal approval. Propriety prevents me from sharing what I think of 
this practice here.

The case in question lies somewhere in the middle, but IMHO a -1 would 
have been perfectly adequate, and I find it extremely improbable that 
another core reviewer would have merged the patch over that objection. 
If you're really worried though, the appropriate thing is to set the 
Approved- (WIP) flag. This sends a clear signal that the patch is not 
ready to merge (this may even be enforced by Gerrit, not 100% sure), but 
is also not sticky so it will drop off on the next patchset. This avoids 
all of the problems with the patch going into review purgatory and being 
blocked by the reviewer being on vacation.

cheers,
Zane.