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

Itamar Heim iheim at redhat.com
Wed Mar 12 15:52:25 UTC 2014 

On 03/12/2014 04:57 PM, Eli Mesika wrote:
>
>
> ----- Original Message -----
>> From: "David Caro" <dcaroest at redhat.com>
>> To: "Itamar Heim" <iheim at redhat.com>
>> Cc: "Maor Lipchuk" <mlipchuk at redhat.com>, users at ovirt.org, "Tomasz Kołek" <tomasz-kolek at o2.pl>, "infra"
>> <infra at ovirt.org>
>> 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 at redhat.com>
>>>>>>> To: "Eyal Edri" <eedri at redhat.com>, "Tomasz Kołek"
>>>>>>> <tomasz-kolek at o2.pl>, users at ovirt.org, "infra" <infra at ovirt.org>
>>>>>>> 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 at 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 at 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 at 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 at redhat.com
>> Web: www.redhat.com
>> RHT Global #: 82-62605
>>
>>
>> _______________________________________________
>> Infra mailing list
>> Infra at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/infra
>>

More information about the Infra mailing list