[Development] Speeding up the review process (was: PostgreSQL cross compile for Pi)

[Jedrzej Nowacki]

On piątek, 13 października 2017 13:04:46 CEST Viktor Engelmann wrote:
> On the [Interest] mailing list there was a discussion about the
> review-process taking to long and we also had multiple discussions about
> that at the world summit. I have complained about this myself, so I
> would like to start a new thread and collect your thoughts and ideas on
> how to improve the situation.
> 
> My suggestions would be
> 
>  1. Allow approving your own commits under certain circumstances. examples:
>      1. when you only changed minor things like a variable name (except
>         in public API), a string, a comment or qdoc entry
>      2. when you already had a +2 and only changed minor things like a
>         variable name, a string, a comment or qdoc entry (or more
>         generally: when you already had a +2 and only did something that
>         is also on this list :-D )
>      3. when you only increased a timeout in an autotest. Increasing
>         timeouts is a safe change - it can only solve false negatives
>         and false positives, but not create false positives or false
>         negatives.
>      4. or more general: when you only made an autotest harder to pass -
>         like adding a Q_VERIFY, Q_COMPARE, etc.
>      5. when the change is something auto-generated - like you just
>         updated plugins.qmltypes using qmlplugindump
>      6. when you only changed something in accordance to the guidelines
>         - like Q_DECL_OVERRIDE -> override
>      7. when you have a certain count of +1's from people who have
>         approver rights
That doesn't solve the problem, no brainers are getting review quite quickly. 
Some more detailed comments below:
 - In 2, you would need to amend the commit message too.
 - 3 is  at best controversial, do not increase timeouts, rewrite the test 
instead.
 - I'm not convinced with 4, it may happen that one is enforcing wrong or 
invalid behaviour.
 - I'm afraid that as a result of 7 people will not give +1s anymore :-)
Review as a process is good, it may be annoying but it is proven to increase 
quality of the code.

>  2. Towards that last point: I think many of us are afraid to get blamed
>     for +2'ing something that causes problems later (introduces a new
>     bug or so), but as far as I have seen, nobody gets blamed for such
>     problems, so we should not be THAT afraid of approving something.
>     Also, don't forget that there is still the CI to get past!
I like how you believe in CI, I share it, but check our test coverage, we are 
far from perfect.

>  3. Remember that brain-cycles are far more expensive than CPU cycles -
>     so when in doubt, rather test-run something on the CI than make a
>     human think about whether the CI "would" accept it. If that causes
>     CI outages, we need to buy more CI machines. It is just a naive
>     fallacy to "save" CI expenses by assigning the CI's work to
>     employees who are much more expensive.
The sentence suggests that we have code review process to cut CI expenses, 
that is simply not true. Anyway, it is not that easy as you wrote there are 
limits of scalability. 

>  4. I don't think we need to be as paranoid towards contributions from
>     our own employees as we need to be towards external contributions.
I do not agree. An employer name does not matter.

>  5. Set a deadline for criticism on the general approach to a change.
>     Too often I have had the situation that I uploaded a patch, then we
>     debated the qdoc entries, variable names, method names, etc FOR
>     MONTHS - and when I thought we were finally wrapping up and I could
>     soon submit it, someone else chimes in and says "this should be done
>     completely differently". Even if the person is right - they should
>     have said that months earlier - before I wasted all that valuable
>     time on variable names, compliance with qdoc guidelines, etc.
>     In earlier discussions I have been told that such a deadline would
>     have to be long, because someone who might have an opinion might be
>     on vacation. IMHO, this doesn't count, because a) you can still make
>     an exception to the rule in such a situation and b) by that logic
>     you should never approve anything, because we also might hire a new
>     employee in 10 years who might have an opinion.
I agree that such situation should not happen, it really should be avoided. I 
have experienced that few times and I know how annoying and demotivating it 
is. In the same time it can not be an excuse to commit a broken code.


I do agree that it takes to long to pass code review, but I do not think that 
giving up on review is a good idea.

Cheers,
  Jędrek