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

----- 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.

> >> >> >>>>>>>>> _______________________________________________ >>>>>>>>> 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 >