<div dir="ltr"><div><div>Hello<br><br>Considering the Testing delay, I was never part of the old system, and I always had to wait for the whole buildbot suite to finish.<br></div>My work flow is I begin my day by checking cdash results and taking action upen these results.<br></div>No more forgotten MR this way.<br><br></div><div class="gmail_extra"><br clear="all"><div><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr">Mathieu Westphal<br></div></div></div>
<br><div class="gmail_quote">On Thu, Jul 7, 2016 at 9:00 PM, Marcus D. Hanwell <span dir="ltr"><<a href="mailto:marcus.hanwell@kitware.com" target="_blank">marcus.hanwell@kitware.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">For what it's worth I have never thought I assumed responsibility for<br>
a merge/pull request I approved/merged for another contributor with<br>
merge rights. I think for new contributors you need to be willing to<br>
mentor/assume a little of that responsibility, but that is part of<br>
building trust, and if they fail to follow up you are less likely to<br>
take their patches in future.<br>
<br>
This is version control, and if you accept something and it totally<br>
backfires we can revert changes, and on occasion I have. Simplifying<br>
sounds good to me, and others can say looks OK but someone who knows<br>
it better should take a look.<br>
<br>
In my opinion one of the other things lacking in VTK is someone owning<br>
a particular module, such that if things are broken/in need of review<br>
the buck stops with them. This makes things a little nebulous at<br>
times, Qt, the Linux kernel and countless others tend to distribute<br>
responsibility for modules, subsystems, etc.<br>
<br>
I think the buildbots have been very slow when I have worked on<br>
changes, and it can be an issue that by the time results are back you<br>
have switched on to other tasks. We had lower coverage but faster<br>
response when only 3 dashboards were required (continuous, and later<br>
Gerrit with CDash@Home). Hopefully this can be improved in future,<br>
such that we could request a branch be tested and see the results<br>
within an hour or so.<br>
<br>
It is tough balancing having enough process that things are managed,<br>
and not so much that it is just too hard to get anything in. I agree<br>
with Berk that if we stick with +1/+2 then they should be additive and<br>
two +1s = a +2.<br>
<div class="HOEnZb"><div class="h5"><br>
On Thu, Jul 7, 2016 at 2:46 PM, Berk Geveci <<a href="mailto:berk.geveci@kitware.com">berk.geveci@kitware.com</a>> wrote:<br>
> I fully agree with Dave & Ken. One minor (not passionate) defense for +1/+2<br>
> is that I may review a piece of code as a non-expert and want to express my<br>
> support but not in full confidence of its correctness. But at the end, the<br>
> process doesn't matter since I can always write instead "I like this code<br>
> but I am not an expert in this."<br>
><br>
> By the way, if we stick with +1, I suggest that multiple +1s mean +2.<br>
><br>
> On Thu, Jul 7, 2016 at 2:32 PM, David E DeMarle <<a href="mailto:dave.demarle@kitware.com">dave.demarle@kitware.com</a>><br>
> wrote:<br>
>><br>
>><br>
>>> This is the real reason people are hesitant to +2. If we strengthen<br>
>>> the meaning of +1 then people will just say "LGTM" or something else<br>
>>> to "approve" without taking responsibility. Posting +1 is a common<br>
>>> convention for voting and should not be given stronger meaning.<br>
>><br>
>><br>
>> How widespread is the non-binary and easier to misinterpret +1/+2 system?<br>
>> I had to explain it to someone recently and they looked at me really funny.<br>
>><br>
>>><br>
>>> The real problem we'd like to address is stagnation of MRs that<br>
>>> are ready but not merged. The syntax for approving is not very<br>
>>> important.<br>
>><br>
>><br>
>> Yes it won't solve the problem, but I think this cultural change will help<br>
>> a tad.<br>
>><br>
>>> We should identify reasons MRs stagnate and address<br>
>>> them directly:<br>
>><br>
>><br>
>> Agree.<br>
>><br>
>>><br>
>>> * It may be a governance problem. No one has responsibility<br>
>>> to ensure everything that is ready gets merged.<br>
>><br>
>><br>
>> Agree.<br>
>><br>
>><br>
>> * I think the big thing is the time problem Will mentioned. Each merge<br>
>> request adds work to uninvolved developers. If our standard is that we only<br>
>> give approval after hours to days of thorough review in the general case<br>
>> then something is broken. If a cursory review says that probably isn't<br>
>> necessary than just give the approval and trust the author to follow<br>
>> through. If it looks like that is necessary than just day so rather than<br>
>> giving a whishy washy +1.<br>
>><br>
>> The +1=="LGTM but don't merge" adds nothing of value.<br>
>><br>
>> * I've also been convinced that the reviewer should not be responsible for<br>
>> monitoring the dashboards at all. Except in the case of newbie authors who<br>
>> don't know how to monitor the dashboards, we should trust each others<br>
>> capabilities and make the original author solely responsible. The important<br>
>> exception is when the original author is a newbie who probably isn't<br>
>> familiar with the dashboards.<br>
>><br>
>><br>
>><br>
>> David E DeMarle<br>
>> Kitware, Inc.<br>
>> R&D Engineer<br>
>> 21 Corporate Drive<br>
>> Clifton Park, NY 12065-8662<br>
>> Phone: <a href="tel:518-881-4909" value="+15188814909">518-881-4909</a><br>
>><br>
>> On Thu, Jul 7, 2016 at 1:41 PM, Berk Geveci <<a href="mailto:berk.geveci@kitware.com">berk.geveci@kitware.com</a>><br>
>> wrote:<br>
>>><br>
>>> If there is new functionality, I do not approve without testing &<br>
>>> documentation. If it's a bug fix, I may ask if there is a test that would<br>
>>> fail without the fix.<br>
>>><br>
>>> On Thu, Jul 7, 2016 at 1:38 PM, Will Schroeder<br>
>>> <<a href="mailto:will.schroeder@kitware.com">will.schroeder@kitware.com</a>> wrote:<br>
>>>><br>
>>>> :-) okay then in order to cross the road, I'll insist more strongly on<br>
>>>> good tests and documentation<br>
>>>><br>
>>>> On Thu, Jul 7, 2016 at 1:27 PM, Berk Geveci <<a href="mailto:berk.geveci@kitware.com">berk.geveci@kitware.com</a>><br>
>>>> wrote:<br>
>>>>><br>
>>>>> > Maybe it's because I'm a chicken<br>
>>>>><br>
>>>>> +1<br>
>>>>><br>
>>>>> On Thu, Jul 7, 2016 at 10:10 AM, Will Schroeder<br>
>>>>> <<a href="mailto:will.schroeder@kitware.com">will.schroeder@kitware.com</a>> wrote:<br>
>>>>>><br>
>>>>>> It's a time problem: I see chunks of code which at a superficial level<br>
>>>>>> seem fine and I'm willing to give it a +1. The problem with a +2 is that<br>
>>>>>> issuing such a score has implications that I've jumped into the code at a<br>
>>>>>> deep level and can vouch for it. This can occasionally mean hours or days of<br>
>>>>>> work hence the delay. Maybe it's because I'm a chicken and should have faith<br>
>>>>>> (or otherwise ensure) that there is enough testing to proof it out, but I<br>
>>>>>> feel reluctant in some cases to give a +2 without enough due diligence which<br>
>>>>>> takes time I often don't have. Besides more testing, one thing that might<br>
>>>>>> help would be more "documentation" providing me with the confidence that<br>
>>>>>> important issues have been considered and accounted for.....<br>
>>>>>><br>
>>>>>> Best,<br>
>>>>>> W<br>
>>>>>><br>
>>>>>><br>
>>>>>> On Thu, Jul 7, 2016 at 9:56 AM, Brad King <<a href="mailto:brad.king@kitware.com">brad.king@kitware.com</a>><br>
>>>>>> wrote:<br>
>>>>>>><br>
>>>>>>> On 07/07/2016 08:56 AM, David E DeMarle wrote:<br>
>>>>>>> > In either case both authors and reviewers are responsible for<br>
>>>>>>> > watching the dashboards and addressing issues that come up<br>
>>>>>>> > afterward.<br>
>>>>>>><br>
>>>>>>> This is the real reason people are hesitant to +2. If we strengthen<br>
>>>>>>> the meaning of +1 then people will just say "LGTM" or something else<br>
>>>>>>> to "approve" without taking responsibility. Posting +1 is a common<br>
>>>>>>> convention for voting and should not be given stronger meaning.<br>
>>>>>>><br>
>>>>>>> The real problem we'd like to address is stagnation of MRs that<br>
>>>>>>> are ready but not merged. The syntax for approving is not very<br>
>>>>>>> important. We should identify reasons MRs stagnate and address<br>
>>>>>>> them directly:<br>
>>>>>>><br>
>>>>>>> * It may be a governance problem. No one has responsibility<br>
>>>>>>> to ensure everything that is ready gets merged.<br>
>>>>>>><br>
>>>>>>> * It may be a workflow problem, like waiting for buildbot results<br>
>>>>>>> to approve and then forgetting. The new workflow:buildbots<br>
>>>>>>> label may help here.<br>
>>>>>>><br>
>>>>>>> -Brad<br>
>>>>>>><br>
>>>>>>> _______________________________________________<br>
>>>>>>> Powered by <a href="http://www.kitware.com" rel="noreferrer" target="_blank">www.kitware.com</a><br>
>>>>>>><br>
>>>>>>> Visit other Kitware open-source projects at<br>
>>>>>>> <a href="http://www.kitware.com/opensource/opensource.html" rel="noreferrer" target="_blank">http://www.kitware.com/opensource/opensource.html</a><br>
>>>>>>><br>
>>>>>>> Search the list archives at:<br>
>>>>>>> <a href="http://markmail.org/search/?q=vtk-developers" rel="noreferrer" target="_blank">http://markmail.org/search/?q=vtk-developers</a><br>
>>>>>>><br>
>>>>>>> Follow this link to subscribe/unsubscribe:<br>
>>>>>>> <a href="http://public.kitware.com/mailman/listinfo/vtk-developers" rel="noreferrer" target="_blank">http://public.kitware.com/mailman/listinfo/vtk-developers</a><br>
>>>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>> --<br>
>>>>>> William J. Schroeder, PhD<br>
>>>>>> Kitware, Inc. - Building the World's Technical Computing Software<br>
>>>>>> 28 Corporate Drive<br>
>>>>>> Clifton Park, NY 12065<br>
>>>>>> <a href="mailto:will.schroeder@kitware.com">will.schroeder@kitware.com</a><br>
>>>>>> <a href="http://www.kitware.com" rel="noreferrer" target="_blank">http://www.kitware.com</a><br>
>>>>>> <a href="tel:%28518%29%20881-4902" value="+15188814902">(518) 881-4902</a><br>
>>>>>><br>
>>>>>> _______________________________________________<br>
>>>>>> Powered by <a href="http://www.kitware.com" rel="noreferrer" target="_blank">www.kitware.com</a><br>
>>>>>><br>
>>>>>> Visit other Kitware open-source projects at<br>
>>>>>> <a href="http://www.kitware.com/opensource/opensource.html" rel="noreferrer" target="_blank">http://www.kitware.com/opensource/opensource.html</a><br>
>>>>>><br>
>>>>>> Search the list archives at:<br>
>>>>>> <a href="http://markmail.org/search/?q=vtk-developers" rel="noreferrer" target="_blank">http://markmail.org/search/?q=vtk-developers</a><br>
>>>>>><br>
>>>>>> Follow this link to subscribe/unsubscribe:<br>
>>>>>> <a href="http://public.kitware.com/mailman/listinfo/vtk-developers" rel="noreferrer" target="_blank">http://public.kitware.com/mailman/listinfo/vtk-developers</a><br>
>>>>>><br>
>>>>>><br>
>>>>><br>
>>>><br>
>>>><br>
>>>><br>
>>>> --<br>
>>>> William J. Schroeder, PhD<br>
>>>> Kitware, Inc. - Building the World's Technical Computing Software<br>
>>>> 28 Corporate Drive<br>
>>>> Clifton Park, NY 12065<br>
>>>> <a href="mailto:will.schroeder@kitware.com">will.schroeder@kitware.com</a><br>
>>>> <a href="http://www.kitware.com" rel="noreferrer" target="_blank">http://www.kitware.com</a><br>
>>>> <a href="tel:%28518%29%20881-4902" value="+15188814902">(518) 881-4902</a><br>
>>><br>
>>><br>
>>><br>
>>> _______________________________________________<br>
>>> Powered by <a href="http://www.kitware.com" rel="noreferrer" target="_blank">www.kitware.com</a><br>
>>><br>
>>> Visit other Kitware open-source projects at<br>
>>> <a href="http://www.kitware.com/opensource/opensource.html" rel="noreferrer" target="_blank">http://www.kitware.com/opensource/opensource.html</a><br>
>>><br>
>>> Search the list archives at: <a href="http://markmail.org/search/?q=vtk-developers" rel="noreferrer" target="_blank">http://markmail.org/search/?q=vtk-developers</a><br>
>>><br>
>>> Follow this link to subscribe/unsubscribe:<br>
>>> <a href="http://public.kitware.com/mailman/listinfo/vtk-developers" rel="noreferrer" target="_blank">http://public.kitware.com/mailman/listinfo/vtk-developers</a><br>
>>><br>
>>><br>
>><br>
><br>
><br>
> _______________________________________________<br>
> Powered by <a href="http://www.kitware.com" rel="noreferrer" target="_blank">www.kitware.com</a><br>
><br>
> Visit other Kitware open-source projects at<br>
> <a href="http://www.kitware.com/opensource/opensource.html" rel="noreferrer" target="_blank">http://www.kitware.com/opensource/opensource.html</a><br>
><br>
> Search the list archives at: <a href="http://markmail.org/search/?q=vtk-developers" rel="noreferrer" target="_blank">http://markmail.org/search/?q=vtk-developers</a><br>
><br>
> Follow this link to subscribe/unsubscribe:<br>
> <a href="http://public.kitware.com/mailman/listinfo/vtk-developers" rel="noreferrer" target="_blank">http://public.kitware.com/mailman/listinfo/vtk-developers</a><br>
><br>
><br>
_______________________________________________<br>
Powered by <a href="http://www.kitware.com" rel="noreferrer" target="_blank">www.kitware.com</a><br>
<br>
Visit other Kitware open-source projects at <a href="http://www.kitware.com/opensource/opensource.html" rel="noreferrer" target="_blank">http://www.kitware.com/opensource/opensource.html</a><br>
<br>
Search the list archives at: <a href="http://markmail.org/search/?q=vtk-developers" rel="noreferrer" target="_blank">http://markmail.org/search/?q=vtk-developers</a><br>
<br>
Follow this link to subscribe/unsubscribe:<br>
<a href="http://public.kitware.com/mailman/listinfo/vtk-developers" rel="noreferrer" target="_blank">http://public.kitware.com/mailman/listinfo/vtk-developers</a><br>
<br>
</div></div></blockquote></div><br></div>