Open Stack

On 28/02/15 09:02, John Griffith wrote:
> 
> 
> On Fri, Feb 27, 2015 at 5:40 PM, Stefano Maffulli <stefano at openstack.org
> <mailto:stefano at openstack.org>> wrote:
> 
>     I'm not expressing myself cleary enough. I don't advocate for the
>     removal of anything because I like pretty charts. I'm changing the
>     subject to be even more clear.
> 
>     On Fri, 2015-02-27 at 13:26 -0800, James E. Blair wrote:
>     > I am asking you to please independently remove changes that you don't
>     > think should be considered from your metrics.
> 
>     I'm saying that the reports have indicators that seem to show struggle.
>     In a previous message Kevin hinted that probably a reason for those bad
>     looking numbers was due to "a lot of reviews that appear to have been
>     abandoned". This doesn't seem the case because some projects have a
>     habit of 'purging'.
> 
>     I have never explicitly ordered developers to purge anything. If their
>     decision to purge is due to the numbers they may have seen on the
>     reports I'd like to know.
> 
>     That said, the problem that the reports highlight remains confirmed so
>     far: contributors seem to be left in some cases hanging, for many many
>     days, *after a comment* and they don't come back.
> 
>     > I think abandoning changes so that the metrics look the way we
>     want is a
>     > terrible experience for contributors.
> 
>     I agree, that should not be a motivator. Also, after chatting with you
>     on IRC I tend to think that instead of abandoning the review we should
>     highlight them and have humans act on them. Maybe build a dashboard of
>     'old stuff' to periodically sift through and see if there are things
>     worth picking up again or to ping the owner or something else managed by
>     humans.
> 
>     I happened to have found one of such review automatically closed that
>     could have been fixed with a trivial edit in commit message instead:
> 
>     https://review.openstack.org/#/c/98735/
> 
>     (that owner had a bunch of auto-abandoned patches
>     https://review.openstack.org/#/q/owner:%22Mh+Raies+%253Craiesmh08%
>     2540gmail.com%253E%22,n,z
>     <https://review.openstack.org/#/q/owner:%22Mh+Raies+%253Craiesmh08%
>     2540gmail.com%253E%22,n,z>). I have made a note to reach out to him and
>     get more anecdotes.
> 
>     > Especially as it appears some projects, such as nova, are in a
>     position
>     > where they are actually leaving -2 votes on changes which will not be
>     > lifted for 2 or 3 months.  That means that if someone runs a
>     script like
>     > Sean's, these changes will be abandoned, yet there is nothing that the
>     > submitter can do to progress the change in the mean time.  Abandoning
>     > such a review is making an already bad experience for contributors
>     even
>     > worse.
> 
>     this sounds like a different issue :(
> 
> 
>     __________________________________________________________________________
>     OpenStack Development Mailing List (not for usage questions)
>     Unsubscribe:
>     OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>     <http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe>
>     http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 
> ​
> For what it's worth, at one point the Cinder project setup an
> auto-abandon job that did purge items that had a negative mark either
> from a reviewer or from Jenkins and had not been updated in over two
> weeks.  This had absolutely nothing to do with metrics or statistical
> analysis of the project.  We simply had a hard time dealing with patches
> that the submitter "didn't care about".  If somebody takes the time to
> review a patch, then I don't think it's too much to ask that the
> submitter respond to questions or comments within a two week period. 
> Note, the auto purge in our case only purged items that had no updates
> or activity at all.
> 
> We were actually in a position where we had patches that were submitted,
> failed unit tests in the gate (valid failures that occurred 100% of the
> time) and had sat for an entire release without the submitter ever
> updating the patch. I don't think it's unreasonable at all to abandon
> these and remove them from the queue.  I don't think this is a bad
> thing, I think it's worse to leave them as active when they're
> bit-rotted and the submitter doesn't even care about them any longer. 
> The other thing is, those patches are "still there", they can still be
> accessed and reinstated.
> 
> There's a lot of knocks against core teams regarding time to review and
> keeping up with the work load.. that's fine, but at the same time if you
> submit something you should follow through on it and respond to comments
> or test failures in a timely manner.  Also there should be some scaling
> factor here; if a patch that needs updating should be expected to be
> able to sit in the queue for a month for example, then we should give
> one month for each reviewer; so minimum of three months for a +1, +2 and +A.
> 
> I don't think it's reasonable to say "hey, you all have to review faster
> and get more done" and then also say "by the way, you need to babysit
> and reach out and contact owners of patches that have been idle for long
> periods".  Especially considering MANY of the patches in Cinder at least
> that end up falling into this category are from folks that aren't on IRC
> and do not have public email addresses in LaunchPad.
> 
> Just providing another perspective.


It's a good perspective John - thank you for your clear writing. I'd
just like to expand on your final paragraph in the hope that we might be
able to inch closer to what I fully realise is likely a pipe dream :)

As you've acknowledged, there's perhaps a small percentage of people who
are in the auto-abandon stage that are a bit different. It's great if
you're Jay Bryant - it can help you clean things that you really have
walked away from. However, there's some number of those people for whom
the reason is not so much they "don't care" about it, but because they
honestly don't have an idea on how to move forward.

The reasons for this are diverse, but like you mention - not being on
IRC, or on mailing lists, or having some kind of email address - means
that for these people, they're quite unlikely to be helped.

It's a fact that our core reviewers don't have time to mentor more than
a handful of people. Perhaps, though, we can code something up to ease
the current process and actively encourage people to seek help from the
wider community.

What about something like:

If {patchset test results are negative} and {no comment from submitter
on patchset} and  {time > 1 weeks} --> Post a message:

"""
Hi!

It looks like you might be having some trouble with fixing up that
failing test result. Don't worry, we're here to help.

Please join us on the project IRC channel, link to this review URL and
ask for help with passing the tests. No need to ask if you can ask -
just explain your problems, and someone will hopefully respond :)

Alternatively, post a message on the Development mailing list with your
project name in the subject line eg "[cinder] Help with tests on review
12345", and tell us in specific detail which bits you need help with.

In case we need to get in touch with you make sure that your email
address in the review is correct, and also available via launchpad.

Note: if you don't want to continue with this patch, you may just click
the 'Abandon' button to let us know. If we don't hear from you, this
patch may be automatically marked as abandoned.


https://wiki.openstack.org/wiki/IRC
https://wiki.openstack.org/wiki/MailingLists#Future_Development
https://launchpad.net/people/+me/+editemails

"""


Probably needs a bunch of queries to work out the right variables to
trigger on: is a week too short? is it just if negative tests? Do we
want them on the ML at all? Is it possible to do a version for
non-test-related -1s?

However, do you think something like this could be useful for those
fraction of folks?



Regards,


Tom