[openstack-dev] [all][infra][tc][ptl] Scaling up code review process (subdir cores)

Thierry Carrez thierry at openstack.org
Wed Jun 3 13:00:53 UTC 2015


So yeah, that's precisely what we discussed at the cross-project
workshop about In-team scaling in Vancouver (led by Kyle and myself).
For those not present, I invite you to read the notes:

https://etherpad.openstack.org/p/liberty-cross-project-in-team-scaling

The conclusion was to explore splitting review areas and building trust
relationships. Those could happen:

- along architectural lines (repo splits)
- along areas of expertise with implicit trust to not review anything else

... which is precisely what you seem to oppose.

Boris Pavlovic wrote:
> *- Why not splitting repo/plugins?*
> 
>   I don't want to make "architectural" decisions based on "social" or 
>   "not enough good tool for review" issues. 
> 
>   If we take a look at OpenStack that was splited many times: Glance,
> Cinder, ...
>   we will see that there is a log of code duplication that can't be
> removed even after 
>   two or even more years of oslo effort. As well it produce such issues
> like syncing 
>   requirements, requirements in such large bash script like devstack, 
>   there is not std installator, it's quite hard to manage and test it
> and so on.. 
> 
>   That's why I don't think that splitting repo is good "architecture"
> decision - it makes 
>    simple things complicated...

I know we disagree on that one, but I don't think monolithic means
"simpler". Having smaller parts that have a simpler role and explicit
contracts to communicate with other pieces is IMHO better and easier to
maintain.

We shouldn't split repositories when it only results in code
duplication. But whenever we can isolate something that could have a
more dedicated maintenance team, I think that's worth exploring as a
solution to the review scaling issue.

> *- Why not just trust people*
> 
> People get tired and make mistakes (very often). 
> That's why we have blocking CI system that checks patches, 
> That's why we have rule 2 cores / review (sometimes even 3,4,5...)... 

It's not because "we don't trust people" that we have the 2-core rule.
Core reviewers check the desirability and quality of implementation. By
default we consider that if 2 of those agree that a change is sane, it
probably is. The CI system checks something else, and that is that you
don't break everyone or introduce a regression. So you shouldn't be able
to "introduce a bug" that would be so serious that a simple revert would
still be embarrassing. If you can, then you should work on your tests.

I think it's totally fine to give people the ability to +2/approve
generally, together with limits on where they are supposed to use that
power. They will be more careful as to what they approve this way. For
corner cases you can revert.

As an example, Ubuntu development has worked on that trust model for
ages. Once you are a developer, you may commit changes to any package in
the distro. But you're supposed to use that power wisely. You end up
staying away from risky packages and everything you don't feel safe to
approve.

If you can't trust your core reviewers to not approve things that are
outside their comfort zone, I'd argue they should not be core reviewers
in the first place.

-- 
Thierry Carrez (ttx)



More information about the OpenStack-dev mailing list