Re: CF3+4 (was Re: Parallel query execution) - Mailing list pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Jan 20, 2013 at 7:07 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
>> As a junior reviewer, I'd like to know if my main task should be to decide
>> between 1) writing a review convincing you or Tom that your judgement is
>> hasty, or 2) to convince the author that your judgement is correct.
>
>  Even if you as a
> reviewer don't know the answer to those questions, clearly delineating
> the key issues may enable others to comment without having to read and
> understand the whole patch, which can expedite things considerably.

We wouldn't have to convince anyone about a judgement being hasty if
only those making judgement took some time and read the patch before
making those judgements on list (even if doing so is not changing the
judgement, it changes the hasty part already).

Offering advice and making judgments are two very different things. And
I'm lucky enough to have received plenty of very good advice from senior
developers here that seldom did read my patches before offering them.

I'm yet to see good use of anyone's time once a hasty judgement has been
made about a patch. And I think there's no value judgement in calling a
judgement hasty here: it's a decision you took and published *about a
patch* before reading the patch. That's about it.

If you don't have the time to read the patch, offer advice. Anything
more, and you can set a watch to count the cumulative time we are all
loosing on trying to convince each other about something else.

Ok, now you will tell me there's no difference and it's just playing
with words. It's not. Judgement is about how the patch do things, Advice
is about how to do things in general.

Ok, here's an hypothetical example, using some words we tend to never
use here because we are all very polite and concerned about each other
happiness when talking about carefully crafted code:
 Advice:    You don't do things that way, this way is the only one we            will ever accept, because we've been
sweatingblood over            the years to get in a position where it now works.
 
            Hint: it's not talking about the patch content, but what is            supposedly a problem with the patch.
It'seasy to answer            "I'm so happy I didn't actually do it that way".
 
 Judgement: You need to think about the problem you want to solve            before sending a patch, because there's a
holein it too            big for me to be able to count how many bugs are going to            to dance in there. It's
nota patch, it's a quagmire. BTW,            I didn't read it, it stinks too much.
 
            Hint: imagine it was your patch and now you have to try and            convince any other commiter to have
alook at it.
 

Now, I've been reviewing patches in all commit fests but one, and began
reviewing well before the idea of a commit fest even existed. My idea of
a good review is being able to come up with one of only 3 possible
summaries:
 - it looks good, please consider a commit, any remaining work is going   to have to be done by a commiter anyway
 - it's not ready, please fix this and that, think about this use case,   review docs, whatever
 - it's ahead of me, this patch now needs a commiter (or just someone   else) to read it then weigth in. at least it
compiles,follow the   usual code and comment style, the documentation is complete and free   or errors, and I tried it
andit does what it says (perfs, features,   etc etc, bonus points for creative usage and trying to make it   crash).
 

Of course, reviewers don't get much feedback in general, so I can only
hope that guideline is good enough for most commiters.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support