Re: [Users] [GSOC][Gerrit] add potential reviewers - questions

From: "Itamar Heim" <iheim(a)redhat.com&gt; To: "Eli Mesika" <emesika(a)redhat.com&gt;, users(a)ovirt.org Cc: "Maor Lipchuk" <mlipchuk(a)redhat.com&gt;, "Tomasz Kołek" <tomasz-kolek(a)o2.pl&gt;, "infra" <infra(a)ovirt.org&gt; Sent: Wednesday, March 12, 2014 5:52:25 PM Subject: Re: [Users] [GSOC][Gerrit] add potential reviewers - questions On 03/12/2014 04:57 PM, Eli Mesika wrote: > > > ----- Original Message ----- >> From: "David Caro" <dcaroest(a)redhat.com&gt; >> To: "Itamar Heim" <iheim(a)redhat.com&gt; >> Cc: "Maor Lipchuk" <mlipchuk(a)redhat.com&gt;, users(a)ovirt.org, "Tomasz Kołek" >> <tomasz-kolek(a)o2.pl&gt;, "infra" >> <infra(a)ovirt.org&gt; >> Sent: Wednesday, March 12, 2014 11:01:21 AM >> Subject: Re: [Users] [GSOC][Gerrit] add potential reviewers - questions >> >> On Wed 12 Mar 2014 08:16:02 AM CET, Itamar Heim wrote: >>> On 03/11/2014 10:08 PM, Maor Lipchuk wrote: >>>> On 03/11/2014 05:20 PM, Itamar Heim wrote: >>>>> On 03/11/2014 05:14 PM, Eyal Edri wrote: >>>>>> >>>>>> >>>>>> ----- Original Message ----- >>>>>>> From: "Itamar Heim" <iheim(a)redhat.com&gt; >>>>>>> To: "Eyal Edri" <eedri(a)redhat.com&gt;, "Tomasz Kołek" >>>>>>> <tomasz-kolek(a)o2.pl&gt;, users(a)ovirt.org, "infra" <infra(a)ovirt.org&gt; >>>>>>> Sent: Tuesday, March 11, 2014 5:10:54 PM >>>>>>> Subject: Re: [Users] [GSOC][Gerrit] add potential reviewers - >>>>>>> questions >>>>>>> >>>>>>> On 03/11/2014 05:06 PM, Ewoud Kohl van Wijngaarden wrote: >>>>>>>> On Tue, Mar 11, 2014 at 10:37:22AM -0400, Eyal Edri wrote: >>>>>>>>>> Tomasz Kołek wrote: >>>>>>>>>> >>>>>>>>>> I've got a few questions about project description. >>>>>>>>>> Please tell me if my problem's understanding is good or not. >>>>>>>>>> We need to add a few flags/methods to git review module. This >>>>>>>>>> flags >>>>>>>>>> should >>>>>>>>>> allow to add potential reviewers in gerrit. >>>>>>>>>> So: >>>>>>>>>> Let's assume that we've got special flags for this operations. >>>>>>>>>> What's >>>>>>>>>> next? >>>>>>>>>> 1. In gerrit system we need to add special place for potential >>>>>>>>>> reviewers? >>>>>>>>>> 2. Potential reviewers should agree that they want to review? >>>>>>>>>> 3. We can have more than one accepted reviewer? >>>>>>>>> >>>>>>>>> I'm not sure i understood exactly what you mean by 'potential >>>>>>>>> reviewers'. do want gerrit (hook?) to automatically add reviewers >>>>>>>>> to >>>>>>>>> a patch according to the code sent? so in fact you'll have a place >>>>>>>>> somewhere for mapping code & specific developers? >>>>>>>> >>>>>>>> I really like this idea. Gerrit currently requires new users to know >>>>>>>> who >>>>>>>> to add as reviewers, IMHO impeding new contributors. >>>>>>>> >>>>>>>> One relative simple solution would be to look at who recently >>>>>>>> touched >>>>>>>> the files that are being modified and add them as reviewers. This >>>>>>>> can be >>>>>>>> done by looking at the git log for a file. Some pseudo python code >>>>>>>> solution: >>>>>>>> >>>>>>>> reviewers = set() >>>>>>>> >>>>>>>> for modified_file in commit.files: >>>>>>>> reviewers += set(commit.author for commit in >>>>>>>> git.log(modified_file)) >>>>>>>> >>>>>>>> return reviewers >>>>>>>> >>>>>>>> This gives a system that those who touche a file, become the >>>>>>>> maintainer >>>>>>>> for that file. A more complex solution could improve on that and >>>>>>>> limit >>>>>>>> the reviewers added per patch. One can think of limiting to only >>>>>>>> contributions in the last X months, weigh contributions so common >>>>>>>> committers are prefered. It could also combine several methods. >>>>>>>> >>>>>>>> For example to limit to the 5 authors who touched the most files: >>>>>>>> >>>>>>>> reviewers = collections.Counter() # New in python 2.7 >>>>>>>> >>>>>>>> for modified_file in commit.files: >>>>>>>> reviewers += collections.Counter(commit.author for commit in >>>>>>>> git.log(modified_file)) >>>>>>>> >>>>>>>> return [author for author, count in reviewers.most_common(5)] >>>>>>>> >>>>>>>> Since Counter also accepts a dictionary, one could also weigh the >>>>>>>> touched lines per file. Downside there is big whitespace/formatting >>>>>>>> patches can skew the line count. >>>>>>>> >>>>>>>> In short, I think an entire thesis could be written on the optimal >>>>>>>> way >>>>>>>> to determine reviewers but a simple algorithm could do to show the >>>>>>>> method works. >>>>>>>> >>>>>>>> Does this help? > > Maybe it will be worth to use the information we have in Bugzilla here: > > We can browse the BZ that were closed/verified in the last XXX days > Per BZ , we know which patches are involved, who reviewed the patches, > which files were changed, when files were changed and the rank of the > change (number of lines changed) > I believe that from this information we can compose a simple ranking > algorithm that its output will be a list of N potential reviewers for the > patch. > Since we can aggregate the above information for all files related to the > patch we want to add reviewers, we can have this set for the whole patch. > This information should be processed and stored each N days and gerrit will > be able to use it. why are we trying to invent hueristics, instead of declaring clear ownership?

> > >>>>>>>> _______________________________________________ >>>>>>>> Users mailing list >>>>>>>> Users(a)ovirt.org >>>>>>>> http://lists.ovirt.org/mailman/listinfo/users >>>>>>>> >>>>>>> >>>>>>> I think if we do this, we want to make sure we cover per file who is >>>>>>> required to +2 it before we consider it acked. >>>>>>> >>>>>> >>>>>> won't it require maintaining static lists of people per >>>>>> file/path/project? >>>>>> >>>>> >>>>> yes, but considering our project layout, i don't see an alternative. >>>>> (some of the layout could be improved to be path based, rather than >>>>> file >>>>> based) >>>> I think it could be done automatically by analysing the file and see who >>>> mostly changed it recently, since the "owner" of the file might be >>>> dynamic, who ever changed most of it few days ago might be more familiar >>>> with it today >>>> >>>> IMO the algorithm of adding the reviewers should be flexible. >>>> For example, using a folder which will contain files, where each file >>>> implement an algorithm to add the reviewers. >>>> >>>> for instance we can have two files: >>>> 1. Add a reviewers by blame - the contributor which changed recently the >>>> code lines >>>> 2. Add a reviewers by file - the contributor who changed most of the >>>> file recently. >>>> >>>> Each file will implement the functional operation and will output the >>>> reviewers emails. >>>> >>>> The user can then add a new algorithm or change it to be more specific >>>> to its project. >>>> for example the user can add also the maintainers which acked the patch >>>> that was blamed. >>>>> _______________________________________________ >>>>> Users mailing list >>>>> Users(a)ovirt.org >>>>> http://lists.ovirt.org/mailman/listinfo/users >>>> >>> >>> this shouldn't be automatic. we need to clearly define ownership. we >>> can't >>> do >>> this per repo for the engine/vdsm. we can do this per repo for the other >>> repo's probably (though solving the folder/file approach would cover the >>> simpler repos as a private case). >>> >>> yes, it will require some work, maybe some moving around of files to make >>> this >>> easier by folders (topics) which should be relevant anyway. >> >> I think it would easier to maintain if we just have one file at the >> root, instead of having the ownership information distributed >> throughout the files/directories. That way you'll know where to look at >> to check/modify the ownership as opposed to having to walk all the >> files and upper directories. >> >> Also, adding all that ownership logic to gerrit might not be easy, as >> it's not meant to go checking the source of the repositories looking >> for configuration. We might want to take a look (again) to zuul, the >> tool that openstack uses as gateway to trigger jenkins jobs and merge >> patches. >> >>> _______________________________________________ >>> Infra mailing list >>> Infra(a)ovirt.org >>> http://lists.ovirt.org/mailman/listinfo/infra >> >> >> >> -- >> David Caro >> >> Red Hat S.L. >> Continuous Integration Engineer - EMEA ENG Virtualization R&D >> >> Email: dcaro(a)redhat.com >> Web: www.redhat.com >> RHT Global #: 82-62605 >> >> >> _______________________________________________ >> Infra mailing list >> Infra(a)ovirt.org >> http://lists.ovirt.org/mailman/listinfo/infra >> _______________________________________________ Infra mailing list Infra(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/infra