Open Stack

>> - SME reviews the code within SLA, which should be defined per component

Also I would like to add, that I'm not against of metrics, we can collect
metrics, in order to figure out if some improvement in the process helped
to speed up reviews, but asking Cores/SMEs to do the job faster will
definitely
affect quality.

So the suggestion is to collect metrics *only* to prove that some
improvement
in the process helped or didn't help.

On Thu, Aug 27, 2015 at 5:58 PM, Evgeniy L <eli at mirantis.com> wrote:

> Hi Mike,
>
> I have several comments.
>
> >> SLA should be the driver of doing timely reviews, however we can’t
> allow to fast-track code into master suffering quality of review ...
>
> As for me the idea of SLA contradicts to qualitative reviews.
>
> Another thing is I got a bit confused by the difference between Core
> Reviewer and Component Lead,
> aren't those the same persons? Shouldn't every Core Reviewer know the
> architecture, best practises
> and participate in design architecture sessions?
>
> >> - If core reviewer has not landed the code yet, Component Lead merges
> patch within SLA defined (or declines to merge and provides explanation as
> part of review).
>
> For example here as far as I'm concerned Component Lead is Core reviewer,
> since
> he has permissions to merge.
>
> Thanks,
>
>
> On Wed, Aug 19, 2015 at 11:31 AM, Mike Scherbakov <
> mscherbakov at mirantis.com> wrote:
>
>> Hi all,
>> let's discuss code review process in Fuel and what we can improve. For
>> those who want to just have a quick context of this email, please check out
>> presentation slides [5].
>>
>> ** Issues **
>> Depending on a Fuel subproject, I'm aware of two buckets of issues with
>> code review in Fuel:
>> a) It is hard to get code reviewed and merged
>> b) Quality of code review itself could be better
>>
>> First bucket:
>> 1) It is hard to find subject matter experts who can help and core
>> reviewers for the area of code, especially if you are new to the project
>> 2) Contributor sometimes receives contradicting opinions from other
>> reviewers, including cores
>> 3) Assigned / responsible core reviewer is needed for a feature in order
>> to help in architectural negotiations, guiding through, landing the code
>> into master
>> 4) Long wait time for getting code reviewed
>>
>> Quality-related items:
>> 5) Not thorough enough, full review in one shot. For example, reviewer
>> can put "-1" due to missed comma, but do not notice major gap in the code.
>> It leads to many patch sets, and demotivation of contributors
>> 6) Some of the core reviewers decreased their involvement, and so number
>> of reviews has dropped dramatically. However, they still occasionally merge
>> code. I propose to remove these cores, and get them back if their
>> involvement is increased back again (I very rarely merge code, but I'm one
>> of those to be removed from cores). This is standard practice in OpenStack
>> community as well, see Neutron as example [4, line 270].
>> 7) As a legacy of the past, we still have old core reviewers being able
>> to merge code in all Fuel repos. All new cores have core rights only for
>> single repo, which is their primary area of expertise. For example, core
>> team size for fuel-library is adidenko + whole fuel-core group [7]. In
>> fact, there are just 4 "trusted" or real core reviewers in fuel-library,
>> not the whole fuel-core group.
>>
>> These problems are not new to OpenStack and open source in general. You
>> can find discussions about same and similar issues in [1], [2], [3].
>>
>>
>> ** Analysis of data **
>> In order to understand what can be improved, I mined the data at first.
>> Main source of information was stackalytics.com. Please take a look at
>> few graphs on slides 4-7 [5], built based on data from stackalytics. Major
>> conclusions from these graphs:
>> 1) Rather small number of core reviewers (in comparison with overall
>> number of contributors) reviewing 40-60% of patch sets, depending on repo
>> (40% fuel-library, 60% fuel-web). See slide #4.
>> 2) Load on core reviewers in Fuel team is higher in average, if you
>> compare it with some other OpenStack projects. Average load on core
>> reviewer across Nova, Keystone, Neutron and Cinder is 2.5 reviews a day. In
>> Fuel though it is 3.6 for fuel-web and 4.6 for fuel-library. See slide #6.
>> 3) Statistics on how fast feedback on code proposed is provided:
>> - fuel-library: 2095 total reviews in 30 days [13], 80 open reviews,
>> average wait time for reviewer - 1d 1h [12]
>> - fuel-web: 1789 total reviews in 30 days [14], 52 open reviews, average
>> wait time for reviewer - 1d 17h [15]
>>
>> There is no need to have deep analysis on whether we have well defined
>> areas of ownership in Fuel components or not: we don’t have it formally
>> defined, and it’s not documented anywhere. So, finding a right core
>> reviewer can be challenging task for a new contributor to Fuel, and this
>> issue has to be addressed.
>>
>>
>> ** Proposed solution **
>> According to stackalytics, for the whole fuel-group we had 262 reviewers
>> with 24 core reviewers for the past 180 days [19]. I think that these
>> numbers can be considered as high enough in order to think about structure
>> in which code review process would be transparent, understandable and
>> scalable.
>>
>> Let’s first agree on the terminology which I’d like to use. It can take
>> pages of precise definitions, however in this email thread I’d like to
>> focus on code review process more, and hopefully high level description of
>> roles would be enough for now.
>> - Contributor: new contributor, who doesn’t work on Fuel regularly and
>> doesn’t know team structure (or full time Fuel developer, who just started
>> his work on Fuel)
>> - SME: subject matter expert of certain Fuel area of code, which he / she
>> regularly contributes to and reviews code of other contributors into this
>> area. Example: network checker or Nailgun agent.
>> - Core Reviewer: expert in one or few parts of Fuel, who was promoted to
>> Core Reviewers team thanks to the contribution, high rate of quality
>> reviews.
>> - Component Lead: The one who defines architecture of a particular module
>> or component in Fuel, does majority of merges there, resolves conflicts
>> between SMEs and / or contributors in the area of responsibility, if
>> conflicts can’t be resolved by other means. Component Lead has to review
>> all design specs in its parts where his/her component is under change.
>> - Fuel PTL: Project Team Lead in its OpenStack standard definition [8],
>> delegates most of the work to component leads, but helps in cases when
>> resolution of conflicts between component leads is needed. A way to resolve
>> conflicts and clear escalation path should help to resolve issue #2. I’d
>> like to notice, that conflicts in a collaborative work is just normal
>> phenomenon. Please see more on this at [11].
>>
>> Fuel currently lacks formal SMEs and their areas of ownership, and
>> component leads. So my suggestion is to address it separately. Some
>> examples on how it is documented in different projects: OpenStack Rally
>> [20], OpenStack Neutron [4, line 105], Docker [10]. Now, in order to solve
>> some of the issues mentioned at the beginning, we need a structure which
>> would have a leverage for it. According to the data analysis, load on core
>> reviewers is extremely high. I think that first step has to be to figure
>> out a way of offloading some work from them in order to ask for better
>> results. Namely, I suggest to:
>> a) identify component leads out of existing core reviewers
>> b) ensure that component leads for large components like Nailgun or
>> fuel-library don’t run features or major feature development, so they can
>> focus on architecture of component, and majority of thorough core reviewers
>> into component
>>
>> Now, I suggest to go even further and not to expect core reviewers to
>> review patches which have not been yet reviewed by contributors’ peers
>> (please see important of it at [17]), SME or which don’t yet have +1 from
>> CI. In fact, this is the suggestion to adopt dictator-lieutenant delegation
>> workflow [7]. To be precise, I would expect that:
>> - Contributor finds SME to review the code. Ideally, contributor can have
>> his/her peers to help with code review first. Contributor doesn’t bother
>> SME, if CI has -1 on a patch proposed
>> - SME reviews the code within SLA, which should be defined per component
>> - Once SME has reviewed a code, Core Reviewer specialized on a component
>> reviews the code within SLA. Review inbox [16, “Ready for Core Reviewers”]
>> can help to find changesets to be reviewed / merged
>> - If core reviewer has not landed the code yet, Component Lead merges
>> patch within SLA defined (or declines to merge and provides explanation as
>> part of review).
>>
>> SLA should be the driver of doing timely reviews, however we can’t allow
>> to fast-track code into master suffering quality of review, just in order
>> to meet SLA. I suggest to see metrics at every IRC weekly meeting, and
>> based on data - ask for help in review core reviewers from other areas, or
>> reset expectations of contributors / SMEs on how fast patches can be
>> reviewed and merged (redefine SLA).
>>
>> This flow is represented on slides 11-14 [5]. SLAs should solve an issue
>> #4 from the list, and align on expectations. Of course, SLAs defined have
>> to be documented somewhere in public place.
>>
>> In order to have convenient and up to date documentation on who are SMEs
>> and component owners for particular areas of code, I suggest similar schema
>> to Google’s one [9] (if we can trust this source, but I like the idea
>> anyway). For Fuel it can look like the following - each top-level directory
>> of every repository has to have file “MAINTAINERS”, which must have list of
>> SMEs and name of a Component Lead. Now, for every changeset proposed,
>> Jenkins can be used to identify folders affected in order to get list of
>> corresponding SMEs and add them to Gerrit review. This should be convenient
>> notification for only those who maintain a particular area, and not a spam
>> for everyone. Such a change should fully address issue #1 from the list.
>>
>> In order to help feature leads to drive the work, ensure that it can land
>> to master by certain date and manage expectations across components
>> properly, we need to identify for every feature, who is the contact point
>> from core reviewers team in every component under change. This can be
>> Component Lead or it can be delegated to some other trusted core reviewer.
>> It is expected that assigned person will participate in periodic sync ups
>> with feature team, consult how changes should be made in order to align
>> with architecture, and find right SMEs to help with code review and/or
>> expertise when needed. This should fully address issue #3.
>>
>> Quality-related issues #6 and #7 already have suggestions. Issue #5,
>> about doing thorough review at first pass, needs close attention. PTL and
>> Component Leads (once identified) have to have a right to remove members of
>> core team, which do not comply to standards of quality in code review.
>> There is a great checklist [18], which I’d encourage everyone to follow
>> while writing code and doing code reviews. Also, there is a specific Fuel
>> Python-related page [6], which may need to be updated. Accordingly, issues
>> of such a kind with particular examples should be escalated to PTL.
>>
>> Thoughts?
>>
>> [1]
>> http://lists.openstack.org/pipermail/openstack-dev/2015-June/065532.html
>> [2]
>> https://etherpad.openstack.org/p/liberty-cross-project-in-team-scaling
>> [3] http://opensource.com/life/14/5/best-code-review-open-source-projects
>> [4]
>> http://git.openstack.org/cgit/openstack/neutron/tree/doc/source/policies/core-reviewers.rst
>> [5]
>> https://docs.google.com/presentation/d/1HMSovUxujOUwILPSjiuZHqAxo1TujQazA2yGHSsQC6U
>> [6] https://wiki.openstack.org/wiki/Fuel/Code_Review_Rules
>> [7] https://git-scm.com/book/en/v2/Distributed-Git-Distributed-Workflows
>> [8] https://wiki.openstack.org/wiki/PTL_Guide
>> [9]
>> http://www.quora.com/What-is-Googles-internal-code-review-policy-process
>> [10] https://github.com/docker/docker/blob/master/MAINTAINERS
>> [11]
>> https://adtmag.com/articles/2014/12/17/agile-conflict-resolution.aspx
>> [12] http://stackalytics.com/report/reviews/fuel-library/open
>> [13] http://stackalytics.com/report/contribution/fuel-library/30
>> [14] http://stackalytics.com/report/contribution/fuel-web/30
>> [15] http://stackalytics.com/report/reviews/fuel-web/open
>> [16] http://bit.ly/1LjYO4t. Full link is available from Fuel wiki:
>> wiki.openstack.org/wiki/Fuel
>> [17] "You should probably be spending at least a couple of hours on code
>> review every day. Not just because the number of code reviewers on a
>> project has the greatest influence on its velocity, but also because its
>> the best way to start building trust with your fellow contributors. If you
>> can show yourself as thoughtful, committed and diligent through your code
>> reviews, then other code reviewers will be much more inclined to prioritize
>> your patches and less carefully scrutinize your work."
>> https://blogs.gnome.org/markmc/2014/06/06/an-ideal-openstack-developer/
>> [18]
>> https://developer.ibm.com/opentech/2015/03/27/checklist-performing-openstack-code-reviews/
>> [19] http://stackalytics.com/report/contribution/fuel-group/180
>>
>> --
>> Mike Scherbakov
>> #mihgen
>>
>>
>> __________________________________________________________________________
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe:
>> OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20150827/47fc6632/attachment.html>