<div dir="ltr"><div><div><div><div>Hi,<br><br></div>I'm all in for any formalization and automation of review process. The only concern that I see here is about core reviewers involvement metrics. If we succeed in reducing the load on core reviewers, it will mean that core reviewers will do less code reviews. This could lead to core reviewer demotion.<br><br>> - Contributor finds SME to <span>review</span> the code. Ideally, contributor can have his/her peers to help with code <span>review</span> first. Contributor doesn’t bother SME, if CI has -1 on a patch proposed<br><br></div>I like the idea with adding reviewers automatically based on MAINTAINERS file. In such case we can drop this ^^ part of instruction. It would be nice if Jenkins could add reviewers after CI +1, or we can use gerrit dashboard for SMEs to not waste their time on review that has not yet passed CI and does not have +1 from other reviewers.<br><br></div>Regards,<br></div>Alex<br><div><div><div><br> </div></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 19, 2015 at 11:31 AM, Mike Scherbakov <span dir="ltr"><<a href="mailto:mscherbakov@mirantis.com" target="_blank">mscherbakov@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">







Hi all,<br>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].<br><br>** Issues **<br>Depending on a Fuel subproject, I'm aware of two buckets of issues with code review in Fuel:<br>a) It is hard to get code reviewed and merged<br>b) Quality of code review itself could be better<br><br>First bucket:<br>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<br>2) Contributor sometimes receives contradicting opinions from other reviewers, including cores<br>3) Assigned / responsible core reviewer is needed for a feature in order to help in architectural negotiations, guiding through, landing the code into master<br>4) Long wait time for getting code reviewed<br><br>Quality-related items:<br>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<br>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].<br>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.<br><br>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].<br><br><br>** Analysis of data **<br>In order to understand what can be improved, I mined the data at first. Main source of information was <a href="http://stackalytics.com" target="_blank">stackalytics.com</a>. Please take a look at few graphs on slides 4-7 [5], built based on data from stackalytics. Major conclusions from these graphs:<br>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.<br>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.<br>3) Statistics on how fast feedback on code proposed is provided:<br>- fuel-library: 2095 total reviews in 30 days [13], 80 open reviews, average wait time for reviewer - 1d 1h [12]<br>- fuel-web: 1789 total reviews in 30 days [14], 52 open reviews, average wait time for reviewer - 1d 17h [15]<div><br>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.<br><br><br>** Proposed solution **<br>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. <br><br>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.<br>- 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)<br>- 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.<br>- 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.<br>- 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.<br>- 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].<br><br>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:<br>a) identify component leads out of existing core reviewers<br>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<br><br>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:<br>- 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<br>- SME reviews the code within SLA, which should be defined per component<br>- 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<br>- 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).<br><br>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).<br><br>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.<br><br>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.<br><br>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.<br><br>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.<br><br>Thoughts?<br><br>[1] <a href="http://lists.openstack.org/pipermail/openstack-dev/2015-June/065532.html" target="_blank">http://lists.openstack.org/pipermail/openstack-dev/2015-June/065532.html</a><br>[2] <a href="https://etherpad.openstack.org/p/liberty-cross-project-in-team-scaling" target="_blank">https://etherpad.openstack.org/p/liberty-cross-project-in-team-scaling</a><br>[3] <a href="http://opensource.com/life/14/5/best-code-review-open-source-projects" target="_blank">http://opensource.com/life/14/5/best-code-review-open-source-projects</a><br>[4] <a href="http://git.openstack.org/cgit/openstack/neutron/tree/doc/source/policies/core-reviewers.rst" target="_blank">http://git.openstack.org/cgit/openstack/neutron/tree/doc/source/policies/core-reviewers.rst</a> <br>[5] <a href="https://docs.google.com/presentation/d/1HMSovUxujOUwILPSjiuZHqAxo1TujQazA2yGHSsQC6U" target="_blank">https://docs.google.com/presentation/d/1HMSovUxujOUwILPSjiuZHqAxo1TujQazA2yGHSsQC6U</a><br>[6] <a href="https://wiki.openstack.org/wiki/Fuel/Code_Review_Rules" target="_blank">https://wiki.openstack.org/wiki/Fuel/Code_Review_Rules</a><br>[7] <a href="https://git-scm.com/book/en/v2/Distributed-Git-Distributed-Workflows" target="_blank">https://git-scm.com/book/en/v2/Distributed-Git-Distributed-Workflows</a><br>[8] <a href="https://wiki.openstack.org/wiki/PTL_Guide" target="_blank">https://wiki.openstack.org/wiki/PTL_Guide</a><br>[9] <a href="http://www.quora.com/What-is-Googles-internal-code-review-policy-process" target="_blank">http://www.quora.com/What-is-Googles-internal-code-review-policy-process</a><br>[10] <a href="https://github.com/docker/docker/blob/master/MAINTAINERS" target="_blank">https://github.com/docker/docker/blob/master/MAINTAINERS</a><br>[11] <a href="https://adtmag.com/articles/2014/12/17/agile-conflict-resolution.aspx" target="_blank">https://adtmag.com/articles/2014/12/17/agile-conflict-resolution.aspx</a><br>[12] <a href="http://stackalytics.com/report/reviews/fuel-library/open" target="_blank">http://stackalytics.com/report/reviews/fuel-library/open</a><br>[13] <a href="http://stackalytics.com/report/contribution/fuel-library/30" target="_blank">http://stackalytics.com/report/contribution/fuel-library/30</a><br>[14] <a href="http://stackalytics.com/report/contribution/fuel-web/30" target="_blank">http://stackalytics.com/report/contribution/fuel-web/30</a><br>[15] <a href="http://stackalytics.com/report/reviews/fuel-web/open" target="_blank">http://stackalytics.com/report/reviews/fuel-web/open</a><br>[16] <a href="http://bit.ly/1LjYO4t" target="_blank">http://bit.ly/1LjYO4t</a>. Full link is available from Fuel wiki: <a href="http://wiki.openstack.org/wiki/Fuel" target="_blank">wiki.openstack.org/wiki/Fuel</a><br>[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." <a href="https://blogs.gnome.org/markmc/2014/06/06/an-ideal-openstack-developer/" target="_blank">https://blogs.gnome.org/markmc/2014/06/06/an-ideal-openstack-developer/</a><br>[18] <a href="https://developer.ibm.com/opentech/2015/03/27/checklist-performing-openstack-code-reviews/" target="_blank">https://developer.ibm.com/opentech/2015/03/27/checklist-performing-openstack-code-reviews/</a><br>[19] <a href="http://stackalytics.com/report/contribution/fuel-group/180" target="_blank">http://stackalytics.com/report/contribution/fuel-group/180</a><span class="HOEnZb"><font color="#888888"><div><br></div>-- <br><div><div dir="ltr">Mike Scherbakov<br>#mihgen<br><br></div></div>
</font></span></div></div>
<br>__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
<br></blockquote></div><br></div>