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

Fri Jul 8 03:06:21 EDT 2016

Hello

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.
My work flow is I begin my day by checking cdash results and taking action
upen these results.
No more forgotten MR this way.

Mathieu Westphal

On Thu, Jul 7, 2016 at 9:00 PM, Marcus D. Hanwell <
marcus.hanwell at kitware.com> wrote:

> 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
> >
> >
> _______________________________________________
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20160708/f59dd28d/attachment-0001.html>