[vtk-developers] Avoiding MR stagnation (was: rules proposal drop "+1")

Marcus D. Hanwell marcus.hanwell at kitware.com
Thu Jul 7 15:00:42 EDT 2016 

For what it's worth I have never thought I assumed responsibility for
a merge/pull request I approved/merged for another contributor with
merge rights. I think for new contributors you need to be willing to
mentor/assume a little of that responsibility, but that is part of
building trust, and if they fail to follow up you are less likely to
take their patches in future.

This is version control, and if you accept something and it totally
backfires we can revert changes, and on occasion I have. Simplifying
sounds good to me, and others can say looks OK but someone who knows
it better should take a look.

In my opinion one of the other things lacking in VTK is someone owning
a particular module, such that if things are broken/in need of review
the buck stops with them. This makes things a little nebulous at
times, Qt, the Linux kernel and countless others tend to distribute
responsibility for modules, subsystems, etc.

I think the buildbots have been very slow when I have worked on
changes, and it can be an issue that by the time results are back you
have switched on to other tasks. We had lower coverage but faster
response when only 3 dashboards were required (continuous, and later
Gerrit with CDash at Home). Hopefully this can be improved in future,
such that we could request a branch be tested and see the results
within an hour or so.

It is tough balancing having enough process that things are managed,
and not so much that it is just too hard to get anything in. I agree
with Berk that if we stick with +1/+2 then they should be additive and
two +1s = a +2.

On Thu, Jul 7, 2016 at 2:46 PM, Berk Geveci <berk.geveci at kitware.com> wrote:
> I fully agree with Dave & Ken. One minor (not passionate) defense for +1/+2
> is that I may review a piece of code as a non-expert and want to express my
> support but not in full confidence of its  correctness. But at the end, the
> process doesn't matter since I can always write instead "I like this code
> but I am not an expert in this."
>
> By the way, if we stick with +1, I suggest that multiple +1s mean +2.
>
> On Thu, Jul 7, 2016 at 2:32 PM, David E DeMarle <dave.demarle at kitware.com>
> wrote:
>>
>>
>>> This is the real reason people are hesitant to +2.  If we strengthen
>>> the meaning of +1 then people will just say "LGTM" or something else
>>> to "approve" without taking responsibility.  Posting +1 is a common
>>> convention for voting and should not be given stronger meaning.
>>
>>
>> How widespread is the non-binary and easier to misinterpret +1/+2 system?
>> I had to explain it to someone recently and they looked at me really funny.
>>
>>>
>>> The real problem we'd like to address is stagnation of MRs that
>>> are ready but not merged.  The syntax for approving is not very
>>> important.
>>
>>
>> Yes it won't solve the problem, but I think this cultural change will help
>> a tad.
>>
>>> We should identify reasons MRs stagnate and address
>>> them directly:
>>
>>
>> Agree.
>>
>>>
>>> * It may be a governance problem.  No one has responsibility
>>>   to ensure everything that is ready gets merged.
>>
>>
>> Agree.
>>
>>
>> * I think the big thing is the time problem Will mentioned. Each merge
>> request adds work to uninvolved developers. If our standard is that we only
>> give approval after hours to days of thorough review in the general case
>> then something is broken. If a cursory review says that probably isn't
>> necessary than just give the approval and trust the author to follow
>> through. If it looks like that is necessary than just day so rather than
>> giving a whishy washy +1.
>>
>> The +1=="LGTM but don't merge" adds nothing of value.
>>
>> * I've also been convinced that the reviewer should not be responsible for
>> monitoring the dashboards at all. Except in the case of newbie authors who
>> don't know how to monitor the dashboards, we should trust each others
>> capabilities and make the original author solely responsible. The important
>> exception is when the original author is a newbie who probably isn't
>> familiar with the dashboards.
>>
>>
>>
>> David E DeMarle
>> Kitware, Inc.
>> R&D Engineer
>> 21 Corporate Drive
>> Clifton Park, NY 12065-8662
>> Phone: 518-881-4909
>>
>> On Thu, Jul 7, 2016 at 1:41 PM, Berk Geveci <berk.geveci at kitware.com>
>> wrote:
>>>
>>> If there is new functionality, I do not approve without testing &
>>> documentation. If it's a bug fix, I may ask if there is a test that would
>>> fail without the fix.
>>>
>>> On Thu, Jul 7, 2016 at 1:38 PM, Will Schroeder
>>> <will.schroeder at kitware.com> wrote:
>>>>
>>>> :-) okay then in order to cross the road, I'll insist more strongly on
>>>> good tests and documentation
>>>>
>>>> On Thu, Jul 7, 2016 at 1:27 PM, Berk Geveci <berk.geveci at kitware.com>
>>>> wrote:
>>>>>
>>>>> > Maybe it's because I'm a chicken
>>>>>
>>>>> +1
>>>>>
>>>>> On Thu, Jul 7, 2016 at 10:10 AM, Will Schroeder
>>>>> <will.schroeder at kitware.com> wrote:
>>>>>>
>>>>>> It's a time problem: I see chunks of code which at a superficial level
>>>>>> seem fine and I'm willing to give it a +1. The problem with a +2 is that
>>>>>> issuing such a score has implications that I've jumped into the code at a
>>>>>> deep level and can vouch for it. This can occasionally mean hours or days of
>>>>>> work hence the delay. Maybe it's because I'm a chicken and should have faith
>>>>>> (or otherwise ensure) that there is enough testing to proof it out, but I
>>>>>> feel reluctant in some cases to give a +2 without enough due diligence which
>>>>>> takes time I often don't have. Besides more testing, one thing that might
>>>>>> help would be more "documentation" providing me with the confidence that
>>>>>> important issues have been considered and accounted for.....
>>>>>>
>>>>>> Best,
>>>>>> W
>>>>>>
>>>>>>
>>>>>> On Thu, Jul 7, 2016 at 9:56 AM, Brad King <brad.king at kitware.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> On 07/07/2016 08:56 AM, David E DeMarle wrote:
>>>>>>> > In either case both authors and reviewers are responsible for
>>>>>>> > watching the dashboards and addressing issues that come up
>>>>>>> > afterward.
>>>>>>>
>>>>>>> This is the real reason people are hesitant to +2.  If we strengthen
>>>>>>> the meaning of +1 then people will just say "LGTM" or something else
>>>>>>> to "approve" without taking responsibility.  Posting +1 is a common
>>>>>>> convention for voting and should not be given stronger meaning.
>>>>>>>
>>>>>>> The real problem we'd like to address is stagnation of MRs that
>>>>>>> are ready but not merged.  The syntax for approving is not very
>>>>>>> important.  We should identify reasons MRs stagnate and address
>>>>>>> them directly:
>>>>>>>
>>>>>>> * It may be a governance problem.  No one has responsibility
>>>>>>>   to ensure everything that is ready gets merged.
>>>>>>>
>>>>>>> * It may be a workflow problem, like waiting for buildbot results
>>>>>>>   to approve and then forgetting.  The new workflow:buildbots
>>>>>>>   label may help here.
>>>>>>>
>>>>>>> -Brad
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Powered by www.kitware.com
>>>>>>>
>>>>>>> Visit other Kitware open-source projects at
>>>>>>> http://www.kitware.com/opensource/opensource.html
>>>>>>>
>>>>>>> Search the list archives at:
>>>>>>> http://markmail.org/search/?q=vtk-developers
>>>>>>>
>>>>>>> Follow this link to subscribe/unsubscribe:
>>>>>>> http://public.kitware.com/mailman/listinfo/vtk-developers
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> William J. Schroeder, PhD
>>>>>> Kitware, Inc. - Building the World's Technical Computing Software
>>>>>> 28 Corporate Drive
>>>>>> Clifton Park, NY 12065
>>>>>> will.schroeder at kitware.com
>>>>>> http://www.kitware.com
>>>>>> (518) 881-4902
>>>>>>
>>>>>> _______________________________________________
>>>>>> Powered by www.kitware.com
>>>>>>
>>>>>> Visit other Kitware open-source projects at
>>>>>> http://www.kitware.com/opensource/opensource.html
>>>>>>
>>>>>> Search the list archives at:
>>>>>> http://markmail.org/search/?q=vtk-developers
>>>>>>
>>>>>> Follow this link to subscribe/unsubscribe:
>>>>>> http://public.kitware.com/mailman/listinfo/vtk-developers
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> William J. Schroeder, PhD
>>>> Kitware, Inc. - Building the World's Technical Computing Software
>>>> 28 Corporate Drive
>>>> Clifton Park, NY 12065
>>>> will.schroeder at kitware.com
>>>> http://www.kitware.com
>>>> (518) 881-4902
>>>
>>>
>>>
>>> _______________________________________________
>>> Powered by www.kitware.com
>>>
>>> Visit other Kitware open-source projects at
>>> http://www.kitware.com/opensource/opensource.html
>>>
>>> Search the list archives at: http://markmail.org/search/?q=vtk-developers
>>>
>>> Follow this link to subscribe/unsubscribe:
>>> http://public.kitware.com/mailman/listinfo/vtk-developers
>>>
>>>
>>
>
>
> _______________________________________________
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at
> http://www.kitware.com/opensource/opensource.html
>
> Search the list archives at: http://markmail.org/search/?q=vtk-developers
>
> Follow this link to subscribe/unsubscribe:
> http://public.kitware.com/mailman/listinfo/vtk-developers
>
>

More information about the vtk-developers mailing list