Open Stack

Excerpts from Doug Hellmann's message of 2015-04-24 07:23:35 -0700:
> Excerpts from Julien Danjou's message of 2015-04-24 10:14:38 +0200:
> > Hi there,
> > 
> > This is now happening weekly to me now, probably because I write too
> > many patches touching almost all OpenStack projects once a cycle, and
> > I'm really tired of that behavior, so PLEASE:
> > 
> >   *Stop sending Code-Review-1 when asking a question in a patch*
> > 
> > _Sometimes_ there are good reasons to set -1 even when asking a
> > question. For example, when the question is a hint sent to the patch
> > author so that (s)he improves is commit message, a code comment or a
> > piece of code.
> > 
> > But most of the time, if you ask a question because there's something
> > YOU DO NOT KNOW OR UNDERSTAND, do not put a score to a patchset. You
> > don't know the answer, so you have absolutely no right to evaluate a
> > patchset with -1. Just don't set a score, it's OK, and wait for the
> > answer before deciding if the patch is worth [-1..+2].
> > 
> > Thank you for listening, and happy hacking!
> > 
> 
> In defense of those of us asking questions, I'll just point out
> that as a core reviewer I need to be sure I understand the intent
> and wide-ranging ramifications of patches as I review them.  Especially
> in the Oslo code, what appears to be a small local change can have
> unintended consequences when the library gets out into the applications.
> 
> I will often ask questions like, "what is going to happen in X
> situation if we change this default" or "how does this change in
> behavior affect the case where Y happens, which isn't well tested
> in our unit tests." If those details aren't made clear by the commit
> message and comments in the code, I consider that a good reason to
> include a -1 with a request for the author to provide more detail.
> Often these are cases I'm not intimately familiar with, so I ask a
> question rather than saying outright that I think something is
> broken because I expect to learn from the answer but I still have
> doubts that I want to indicate with the -1.
> 

Doug I think those questions fall into the bucket of "leading, relevant
questions." These are real -1 reviews. A change that doesn't have
supporting evidence in the commit message, comments, and/or
documentation, is often a ticking time-bomb that goes off when code is
released into the wild, or sometimes much later when the what-if comes
true.

What I think Julien is frustrated by is not "what happens if ..." but "I
don't understand how this can work" or "can we really do X in library
Y?" type questions, which I've definitely seen too and have had reviews
held up for months because the -1 drops it off many reviewers' radars.

> Most of the time the author has thought about the issues and worked
> out a reason they are not a problem, but they haven't explained
> that anywhere. On the other hand, it is frequently the case that
> someone *hasn't* understood why a change might be bad and the
> question ends up leading to more research and discussion.
> 

Yes, I totally agree. Commit message is a good place to explain "why"
the change is a) needed, and b) safe. Comments and docstrings are also a
good place to explain (b), such as 

'''Mutates state in X

   No serialization is done in this method, ensure all callers lock X
   first'''

But again, asking "how does this new method ensure X mutations are
atomic?" is an awesome leading question, and nobody is frustrated about
those.

So, here's what I think is a decent rule, which we might want to put in
our reviewer checklist. I've submitted a review for wording that I think
might be appropriate.

https://review.openstack.org/177364