Re: [openstack-dev] [Fuel] Code review process in Fuel and related issues
Hi Mike, thanks, now it's clear. On Thu, Sep 3, 2015 at 9:19 AM, Mike Scherbakov wrote: > Thank you all for the feedback. > > > Dims - > > > 1) I'd advise to codify a proposal in fuel-specs under a 'policy' > directory > > I think it's great idea and I'll do it. > > > > 2) We don't have SME terminology, but we do have Maintainers both in > oslo-incubator > > I like "Maintainers" more than SMEs, thank you for suggestion. I'd switch > SME -> Maintainer everywhere. > > > > 3) Is there a plan to split existing repos to more repos? Then each repo > can have a core team (one core team for one repo), PTL takes care of all > repos and MAINTAINERS take care of directories within a repo. That will > line up well with what we are doing elsewhere in the community (essentially > "Component Lead" is a core team which may not be a single person). > > That's the plan, with one difference though. According to you, there is a > core team per repo without a lead identified. In my proposal, I'd like to > ensure that we always choose a lead by the process of voting. I'd like to > have voting process of identifying a component lead. It has to be fair > process. > > > > We do not have a concept of SLA anywhere that i know of, so it will have > to be some kind of social consensus and not a real carrot/stick. > > > As for me the idea of SLA contradicts to qualitative reviews. > > I'm not thinking about carrot or stick here. I'd like to ensure that core > reviewers and component leads are targeted to complete reviews within a > certain time. If it doesn't happen, then patchset needs to be discussed > during IRC meeting if we can delegate some testing, etc. to someone. If > there are many patchsets out of SLA, then we'd consider other changes > (decide to drop something from the release so to free up resources or > something else). > > We had a problem in the past, when we would not pay attention to a patch > proposed by someone before it's being escalated. I'm suggesting a solution > for that problem. SLA is the contract between contributor and reviewer, and > both would have same expectations on how long would it take to review the > patch. Without expectations aligned, contributors can get upset easily. > They may expect that their code will be reviewed and merged within hours, > while it fact it's days. I'm not even talking about patches which can be > forgotten and hang in the queue for months... > > > > 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. > > I expect that there will be a drop in code reviews being done by core > reviewers team. This is the point actually - do less reviews, but do it > more thoroughly. Don't work on patches which have easy mistakes, as those > should be cought by maintainers, before these patches come to the core > reviewer's plate. I don't think though that it will lead to core reviewer > "demotion". Still, you will be doing many reviews - just less than before, > and other who did a little - will do more reviews, potentially becoming > joining a core team later. > > > > 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. > > This is good suggestion. I agree. > > > > AFAIK Boris Pavlovic introduced some scripts > > > in Rally which do basic preliminary check of review message, checking > > > that it's formally correct. > > Thanks Igor, I believe this can be applied as well. > > > > 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? > > Component Lead is being elected by core reviewers team as the lead. So > it's just another core reviewer / architect, but with the right to have a > final word. Also, for large parts like fuel-library / nailgun, I'd expect > this person to be free from any feature-work. For smaller things, like > network verifier, I don't think that we'd need to have dedicated component > owner who will be free from any feature work. > > > Igor K., Evgeny L, did I address your questions regarding SLA and > component lead vs core reviewer? > > Thank you, > > On Wed, Sep 2, 2015 at 9:28 AM Jay Pipes wrote: > >> On 09/02/2015 08:45 AM, Igor Kalnitsky wrote: >> >> I think there's plenty of examples of people in OpenStack projects >> >> that both submit code (and lead features) that also do code review >> >> on a daily basis. >> > >> > * Do these features huge? >> >> Yes. >> >> > * Is their code contribution huge or just small patches? >> >> Both. >> >> > * Did they get to master before FF? >> >> Yes. >> >> > * How many intersecting features OpenStack projects have under >> > development? (since often merge conf
Re: [openstack-dev] [Fuel] Code review process in Fuel and related issues
Thank you all for the feedback. Dims - > 1) I'd advise to codify a proposal in fuel-specs under a 'policy' directory I think it's great idea and I'll do it. > 2) We don't have SME terminology, but we do have Maintainers both in oslo-incubator I like "Maintainers" more than SMEs, thank you for suggestion. I'd switch SME -> Maintainer everywhere. > 3) Is there a plan to split existing repos to more repos? Then each repo can have a core team (one core team for one repo), PTL takes care of all repos and MAINTAINERS take care of directories within a repo. That will line up well with what we are doing elsewhere in the community (essentially "Component Lead" is a core team which may not be a single person). That's the plan, with one difference though. According to you, there is a core team per repo without a lead identified. In my proposal, I'd like to ensure that we always choose a lead by the process of voting. I'd like to have voting process of identifying a component lead. It has to be fair process. > We do not have a concept of SLA anywhere that i know of, so it will have to be some kind of social consensus and not a real carrot/stick. > As for me the idea of SLA contradicts to qualitative reviews. I'm not thinking about carrot or stick here. I'd like to ensure that core reviewers and component leads are targeted to complete reviews within a certain time. If it doesn't happen, then patchset needs to be discussed during IRC meeting if we can delegate some testing, etc. to someone. If there are many patchsets out of SLA, then we'd consider other changes (decide to drop something from the release so to free up resources or something else). We had a problem in the past, when we would not pay attention to a patch proposed by someone before it's being escalated. I'm suggesting a solution for that problem. SLA is the contract between contributor and reviewer, and both would have same expectations on how long would it take to review the patch. Without expectations aligned, contributors can get upset easily. They may expect that their code will be reviewed and merged within hours, while it fact it's days. I'm not even talking about patches which can be forgotten and hang in the queue for months... > 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. I expect that there will be a drop in code reviews being done by core reviewers team. This is the point actually - do less reviews, but do it more thoroughly. Don't work on patches which have easy mistakes, as those should be cought by maintainers, before these patches come to the core reviewer's plate. I don't think though that it will lead to core reviewer "demotion". Still, you will be doing many reviews - just less than before, and other who did a little - will do more reviews, potentially becoming joining a core team later. > 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. This is good suggestion. I agree. > AFAIK Boris Pavlovic introduced some scripts > in Rally which do basic preliminary check of review message, checking > that it's formally correct. Thanks Igor, I believe this can be applied as well. > 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? Component Lead is being elected by core reviewers team as the lead. So it's just another core reviewer / architect, but with the right to have a final word. Also, for large parts like fuel-library / nailgun, I'd expect this person to be free from any feature-work. For smaller things, like network verifier, I don't think that we'd need to have dedicated component owner who will be free from any feature work. Igor K., Evgeny L, did I address your questions regarding SLA and component lead vs core reviewer? Thank you, On Wed, Sep 2, 2015 at 9:28 AM Jay Pipes wrote: > On 09/02/2015 08:45 AM, Igor Kalnitsky wrote: > >> I think there's plenty of examples of people in OpenStack projects > >> that both submit code (and lead features) that also do code review > >> on a daily basis. > > > > * Do these features huge? > > Yes. > > > * Is their code contribution huge or just small patches? > > Both. > > > * Did they get to master before FF? > > Yes. > > > * How many intersecting features OpenStack projects have under > > development? (since often merge conflicts requires a lot of re-review) > > I recognize that Fuel, like devstack, has lots of cross-project > dependencies. That just makes things harder to handle for Fuel, but it's > not a reason to have core reviewers not working on code or non-core > reviewers not doing reviews. > > > * H
Re: [openstack-dev] [Fuel] Code review process in Fuel and related issues
On 09/02/2015 08:45 AM, Igor Kalnitsky wrote: I think there's plenty of examples of people in OpenStack projects that both submit code (and lead features) that also do code review on a daily basis. * Do these features huge? Yes. * Is their code contribution huge or just small patches? Both. * Did they get to master before FF? Yes. * How many intersecting features OpenStack projects have under development? (since often merge conflicts requires a lot of re-review) I recognize that Fuel, like devstack, has lots of cross-project dependencies. That just makes things harder to handle for Fuel, but it's not a reason to have core reviewers not working on code or non-core reviewers not doing reviews. * How often OpenStack people are busy on other activities, such as helping fellas, troubleshooting customers, participate design meetings and so on? Quite often. I'm personally on IRC participating in design discussions, code reviews, and helping people every day. Not troubleshooting customers, though... If so, do you sure they are humans then? :) I can only speak for myself, and that's what I want to say: during 7.0 dev cycle I burned in hell and I don't want to continue that way. I think you mean you "burned out" :) But, yes, I hear you. I understand the pressure that you are under, and I sympathize with you. I just feel that the situation is not an either/or situation, and encouraging some folks to only do reviews and not participate in coding/feature development is a dangerous thing. Best, -jay Thanks, Igor On Wed, Sep 2, 2015 at 3:14 PM, Jay Pipes wrote: On 09/02/2015 03:00 AM, Igor Kalnitsky wrote: It won't work that way. You either busy on writing code / leading feature or doing review. It couldn't be combined effectively. Any context switch between activities requires an extra time to focus on. I don't agree with the above, Igor. I think there's plenty of examples of people in OpenStack projects that both submit code (and lead features) that also do code review on a daily basis. Best, -jay __ 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
Re: [openstack-dev] [Fuel] Code review process in Fuel and related issues
> I think there's plenty of examples of people in OpenStack projects > that both submit code (and lead features) that also do code review > on a daily basis. * Do these features huge? * Is their code contribution huge or just small patches? * Did they get to master before FF? * How many intersecting features OpenStack projects have under development? (since often merge conflicts requires a lot of re-review) * How often OpenStack people are busy on other activities, such as helping fellas, troubleshooting customers, participate design meetings and so on? If so, do you sure they are humans then? :) I can only speak for myself, and that's what I want to say: during 7.0 dev cycle I burned in hell and I don't want to continue that way. Thanks, Igor On Wed, Sep 2, 2015 at 3:14 PM, Jay Pipes wrote: > On 09/02/2015 03:00 AM, Igor Kalnitsky wrote: >> >> It won't work that way. You either busy on writing code / leading >> feature or doing review. It couldn't be combined effectively. Any >> context switch between activities requires an extra time to focus on. > > > I don't agree with the above, Igor. I think there's plenty of examples of > people in OpenStack projects that both submit code (and lead features) that > also do code review on a daily basis. > > Best, > -jay > > > __ > 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
Re: [openstack-dev] [Fuel] Code review process in Fuel and related issues
On 09/02/2015 03:00 AM, Igor Kalnitsky wrote: It won't work that way. You either busy on writing code / leading feature or doing review. It couldn't be combined effectively. Any context switch between activities requires an extra time to focus on. I don't agree with the above, Igor. I think there's plenty of examples of people in OpenStack projects that both submit code (and lead features) that also do code review on a daily basis. Best, -jay __ 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
Re: [openstack-dev] [Fuel] Code review process in Fuel and related issues
It won't work that way. You either busy on writing code / leading feature or doing review. It couldn't be combined effectively. Any context switch between activities requires an extra time to focus on. On Wed, Sep 2, 2015 at 5:46 AM, Tomasz Napierala wrote: >> On 01 Sep 2015, at 03:43, Igor Kalnitsky wrote: >> >> Hi folks, >> >> So basically.. >> >> * core reviewers won't be feature leads anymore >> * core reviewers won't be assigned to features (or at least not full-time) >> * core reviewers will spend time doing review and participate design meetings >> * core reviewers will spend time triaging bugs >> >> Is that correct? >> > > From what I understand, it is not correct. Core reviewer will still do all > this activities in most cases. What we are trying to achieve, is to get > core's attention only to reviews, that are already reviewed by SMEs and > peers. We > hope this will increase the quality of code core reviewers are getting. > > Regards, > -- > Tomasz 'Zen' Napierala > Product Engineering - Poland > > > > > > > > __ > 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
Re: [openstack-dev] [Fuel] Code review process in Fuel and related issues
> On 01 Sep 2015, at 03:43, Igor Kalnitsky wrote: > > Hi folks, > > So basically.. > > * core reviewers won't be feature leads anymore > * core reviewers won't be assigned to features (or at least not full-time) > * core reviewers will spend time doing review and participate design meetings > * core reviewers will spend time triaging bugs > > Is that correct? > >From what I understand, it is not correct. Core reviewer will still do all >this activities in most cases. What we are trying to achieve, is to get core's >attention only to reviews, that are already reviewed by SMEs and peers. We hope this will increase the quality of code core reviewers are getting. Regards, -- Tomasz 'Zen' Napierala Product Engineering - Poland __ 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
Re: [openstack-dev] [Fuel] Code review process in Fuel and related issues
Hi folks, So basically.. * core reviewers won't be feature leads anymore * core reviewers won't be assigned to features (or at least not full-time) * core reviewers will spend time doing review and participate design meetings * core reviewers will spend time triaging bugs Is that correct? Thanks, Igor On Sun, Aug 30, 2015 at 2:29 AM, Tomasz Napierala wrote: >> On 27 Aug 2015, at 07:58, Evgeniy L 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. > > We expect cores to be less loaded after this change, so you guys should have > more time to spend on right reviews, and not minor stuff. We hope this will > also help keeping SLAs. > >> 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? > > Not really. You can have many core reviewers, but there should be one > component lead. Currently, while Fuel is monolithic, we cannot implement it > in technical way. But if we succeed splitting Fuel into smaller projects, > component lead will be responsible for (most likely) one repo. > > Regards, > -- > Tomasz 'Zen' Napierala > Product Engineering - Poland > > > > > > > > > __ > 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
Re: [openstack-dev] [Fuel] Code review process in Fuel and related issues
> On 27 Aug 2015, at 07:58, Evgeniy L 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. We expect cores to be less loaded after this change, so you guys should have more time to spend on right reviews, and not minor stuff. We hope this will also help keeping SLAs. > 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? Not really. You can have many core reviewers, but there should be one component lead. Currently, while Fuel is monolithic, we cannot implement it in technical way. But if we succeed splitting Fuel into smaller projects, component lead will be responsible for (most likely) one repo. Regards, -- Tomasz 'Zen' Napierala Product Engineering - Poland __ 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
Re: [openstack-dev] [Fuel] Code review process in Fuel and related issues
>> - 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 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 < > 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 ** >> Acc
Re: [openstack-dev] [Fuel] Code review process in Fuel and related issues
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 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
Re: [openstack-dev] [Fuel] Code review process in Fuel and related issues
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 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 > 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
Re: [openstack-dev] [Fuel] Code review process in Fuel and related issues
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 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
Re: [openstack-dev] [Fuel] Code review process in Fuel and related issues
Mike, This is a great start. 1) I'd advise to codify a proposal in fuel-specs under a 'policy' directory (obviously as a review in fuel-specs repo) So everyone agrees to the structure of the teams and terminology etc. Example oslo uses a directory to write down some of our decisions. http://git.openstack.org/cgit/openstack/oslo-specs/tree/specs/policy 2) We don't have SME terminology, but we do have Maintainers both in oslo-incubator ( http://git.openstack.org/cgit/openstack/oslo-incubator/tree/MAINTAINERS) and in Rally (https://rally.readthedocs.org/en/latest/project_info.html) So let's use that 3) Is there a plan to split existing repos to more repos? Then each repo can have a core team (one core team for one repo), PTL takes care of all repos and MAINTAINERS take care of directories within a repo. That will line up well with what we are doing elsewhere in the community (essentially "Component Lead" is a core team which may not be a single person). We do not have a concept of SLA anywhere that i know of, so it will have to be some kind of social consensus and not a real carrot/stick. One way is to publish/use data about reviews like stackalytics or russell's site ( http://russellbryant.net/openstack-stats/fuel-openreviews.html) and policy the reviews that drop off the radar during the weekly meetings or something like that. Thanks, Dims On Wed, Aug 19, 2015 at 4:31 AM, Mike Scherbakov 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 fu
[openstack-dev] [Fuel] Code review process in Fuel and related issues
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 com
