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

[Robert Löhning]

Am 13.10.2017 um 14:07 schrieb 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.

First of all I'd like to thank Viktor for figuring out ways to improve
our review process and getting the discussion going.

While it seems that mostly everything has been said about the other
bullet points, I'd like to comment on points 2 and 3. I get the
impression that these encourage people to trust in CI far too much. My
understanding is that the developers writing code should make sure that
everything they wrote does what they expect. If anything is unclear, the
issue should be discussed with others who know more about the affected
code or its influences. At the time someone stages a change at least
this person and the person giving +2 should be completely convinced that
the patch is free of mistakes. They should act as if no CI ever existed.

Of course, as human beings, we all make mistakes and no matter how
convinced someone is of their work results, they might turn out to be
wrong. To address this, we have the CI, we have RTA, we have manual
tests. These are there to find the remaining errors which slip through.
At least this how I see them. Using them as the judge whether a patch is
good or not, is way beyond what they are intended for. Please correct me
if I'm mistaken.

Cheers,
Robert

> 
>>  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