Mike, speaking of automation, AFAIK Boris Pavlovic introduced some scripts in Rally which do basic preliminary check of review message, checking that it's formally correct. It should make life of reviewers a bit easier, you might want to introduce them in Fuel as well, if not yet. Regards, Igor Marnat
On Thu, Aug 27, 2015 at 5:37 PM, Aleksandr Didenko <adide...@mirantis.com> wrote: > Hi, > > 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. > >> - 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 > > 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. > > Regards, > Alex > > > On Wed, Aug 19, 2015 at 11:31 AM, Mike Scherbakov <mscherba...@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-requ...@lists.openstack.org?subject:unsubscribe >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >> > > > __________________________________________________________________________ > OpenStack Development Mailing List (not for usage questions) > Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > __________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
