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

From: "Itamar Heim" <iheim(a)redhat.com&gt; To: "Maor Lipchuk" <mlipchuk(a)redhat.com&gt;, "Alon Bar-Lev" <alonbl(a)redhat.com&gt; Cc: "Eli Mesika" <emesika(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 9:29:28 PM Subject: Re: [Users] [GSOC][Gerrit] add potential reviewers - questions On 03/12/2014 07:29 PM, Maor Lipchuk wrote: > On 03/12/2014 05:57 PM, Alon Bar-Lev wrote: >> >> >> ----- Original Message ----- >>> 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? >> >> Reminder: my source metadata plan that requires cooperation. >> >> Each source and component should have an explicit ownership up into bug >> database. >> >> I won't repeat it now, it is available at archives. >> > I think we are discussing two different issues here: > The first one considers the GSOC project, of adding reviewers > automatically to a patch, this can be done in many different heuristics > depending what the submitter prefers (use blame, maintainers who acked > the patches, bugs, list of owners to a file and so on). that's the point. we should be using heuristics, rather ownership.

this could still be done via a GSOC project for implementing the gerrit logic to do so. > > The second issue is adding and maintain a list of owners by > files/folders in oVirt. > this is a specific use case to be used by the "add potential reviewers" > project. > >>> >>>> >>>> >>>>>>>>>>> _______________________________________________ >>>>>>>>>>> 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 >>> >