Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Matthew Booth]

On 19/06/14 13:22, Mark McLoughlin wrote:
> On Thu, 2014-06-19 at 09:34 +0100, Matthew Booth wrote:
>> On 19/06/14 08:32, Mark McLoughlin wrote:
>>> Hi Armando,
>>>
>>> On Tue, 2014-06-17 at 14:51 +0200, Armando M. wrote:
 I wonder what the turnaround of trivial patches actually is, I bet you
 it's very very small, and as Daniel said, the human burden is rather
 minimal (I would be more concerned about slowing them down in the
 gate, but I digress).
>>>

 I think that introducing a two-tier level for patch approval can only
 mitigate the problem, but I wonder if we'd need to go a lot further,
 and rather figure out a way to borrow concepts from queueing theory so
 that they can be applied in the context of Gerrit. For instance
 Little's law [1] says:

 "The long-term average number of customers (in this context reviews)
 in a stable system L is equal to the long-term average effective
 arrival rate, λ, multiplied by the average time a customer spends in
 the system, W; or expressed algebraically: L = λW."

 L can be used to determine the number of core reviewers that a project
 will need at any given time, in order to meet a certain arrival rate
 and average time spent in the queue. If the number of core reviewers
 is a lot less than L then that core team is understaffed and will need
 to increase.

 If we figured out how to model and measure Gerrit as a queuing system,
 then we could improve its performance a lot more effectively; for
 instance, this idea of privileging trivial patches over longer patches
 has roots in a popular scheduling policy [3] for  M/G/1 queues, but
 that does not really help aging of 'longer service time' patches and
 does not have a preemption mechanism built-in to avoid starvation. 

 Just a crazy opinion...
 Armando

 [1] - http://en.wikipedia.org/wiki/Little's_law
 [2] - http://en.wikipedia.org/wiki/Shortest_job_first
 [3] - http://en.wikipedia.org/wiki/M/G/1_queue
>>>
>>> This isn't crazy at all. We do have a problem that surely could be
>>> studied and solved/improved by applying queueing theory or lessons from
>>> fields like lean manufacturing. Right now, we're simply applying our
>>> intuition and the little I've read about these sorts of problems is that
>>> your intuition can easily take you down the wrong path.
>>>
>>> There's a bunch of things that occur just glancing through those
>>> articles:
>>>
>>>   - Do we have an unstable system? Would it be useful to have arrival 
>>> and exit rate metrics to help highlight this? Over what time period 
>>> would those rates need to be averaged to be useful? Daily, weekly, 
>>> monthly, an entire release cycle?
>>>
>>>   - What are we trying to optimize for? The length of time in the 
>>> queue? The number of patches waiting in the queue? The response 
>>> time to a new patch revision?
>>>
>>>   - We have a single queue, with a bunch of service nodes with a wide 
>>> variance between their service rates, very little in the way of
>>> scheduling policy, a huge rate of service nodes sending jobs back 
>>> for rework, a cost associated with maintaining a job while it sits 
>>> in the queue, the tendency for some jobs to disrupt many other jobs 
>>> with merge conflicts ... not simple.
>>>
>>>   - Is there any sort of natural limit in our queue size that makes the 
>>> system stable - e.g. do people naturally just stop submitting
>>> patches at some point?
>>>
>>> My intuition on all of this lately is that we need some way to model and
>>> experiment with this queue, and I think we could make some interesting
>>> progress if we could turn it into a queueing network rather than a
>>> single, extremely complex queue.
>>>
>>> Say we had a front-end for gerrit which tracked which queue a patch is
>>> in, we could experiment with things like:
>>>
>>>   - a triage queue, with non-cores signed up as triagers looking for 
>>> obvious mistakes and choosing the next queue for a patch to enter 
>>> into
>>>
>>>   - queues having a small number of cores signed up as owners - e.g. 
>>> high priority bugfix, API, scheduler, object conversion, libvirt
>>> driver, vmware driver, etc.
>>>
>>>   - we'd allow for a large number of queues so that cores could aim for 
>>> an "inbox zero" approach on individual queues, something that would 
>>> probably help keep cores motivated.
>>>
>>>   - we could apply different scheduling policies to each of the 
>>> different queues - i.e. explicit guidance for cores about which 
>>> patches they should pick off the queue next.
>>>
>>>   - we could track metrics on individual queues as well as the whole 
>>> network, identifying bottlenecks and properly recognizing which 
>>> reviewers are doing a small number of difficult reviews versus 
>>> those doing a high number of trivial review

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Mark McLoughlin]

On Thu, 2014-06-19 at 09:34 +0100, Matthew Booth wrote:
> On 19/06/14 08:32, Mark McLoughlin wrote:
> > Hi Armando,
> > 
> > On Tue, 2014-06-17 at 14:51 +0200, Armando M. wrote:
> >> I wonder what the turnaround of trivial patches actually is, I bet you
> >> it's very very small, and as Daniel said, the human burden is rather
> >> minimal (I would be more concerned about slowing them down in the
> >> gate, but I digress).
> > 
> >>
> >> I think that introducing a two-tier level for patch approval can only
> >> mitigate the problem, but I wonder if we'd need to go a lot further,
> >> and rather figure out a way to borrow concepts from queueing theory so
> >> that they can be applied in the context of Gerrit. For instance
> >> Little's law [1] says:
> >>
> >> "The long-term average number of customers (in this context reviews)
> >> in a stable system L is equal to the long-term average effective
> >> arrival rate, λ, multiplied by the average time a customer spends in
> >> the system, W; or expressed algebraically: L = λW."
> >>
> >> L can be used to determine the number of core reviewers that a project
> >> will need at any given time, in order to meet a certain arrival rate
> >> and average time spent in the queue. If the number of core reviewers
> >> is a lot less than L then that core team is understaffed and will need
> >> to increase.
> >>
> >> If we figured out how to model and measure Gerrit as a queuing system,
> >> then we could improve its performance a lot more effectively; for
> >> instance, this idea of privileging trivial patches over longer patches
> >> has roots in a popular scheduling policy [3] for  M/G/1 queues, but
> >> that does not really help aging of 'longer service time' patches and
> >> does not have a preemption mechanism built-in to avoid starvation. 
> >>
> >> Just a crazy opinion...
> >> Armando
> >>
> >> [1] - http://en.wikipedia.org/wiki/Little's_law
> >> [2] - http://en.wikipedia.org/wiki/Shortest_job_first
> >> [3] - http://en.wikipedia.org/wiki/M/G/1_queue
> > 
> > This isn't crazy at all. We do have a problem that surely could be
> > studied and solved/improved by applying queueing theory or lessons from
> > fields like lean manufacturing. Right now, we're simply applying our
> > intuition and the little I've read about these sorts of problems is that
> > your intuition can easily take you down the wrong path.
> > 
> > There's a bunch of things that occur just glancing through those
> > articles:
> > 
> >   - Do we have an unstable system? Would it be useful to have arrival 
> > and exit rate metrics to help highlight this? Over what time period 
> > would those rates need to be averaged to be useful? Daily, weekly, 
> > monthly, an entire release cycle?
> > 
> >   - What are we trying to optimize for? The length of time in the 
> > queue? The number of patches waiting in the queue? The response 
> > time to a new patch revision?
> > 
> >   - We have a single queue, with a bunch of service nodes with a wide 
> > variance between their service rates, very little in the way of
> > scheduling policy, a huge rate of service nodes sending jobs back 
> > for rework, a cost associated with maintaining a job while it sits 
> > in the queue, the tendency for some jobs to disrupt many other jobs 
> > with merge conflicts ... not simple.
> > 
> >   - Is there any sort of natural limit in our queue size that makes the 
> > system stable - e.g. do people naturally just stop submitting
> > patches at some point?
> > 
> > My intuition on all of this lately is that we need some way to model and
> > experiment with this queue, and I think we could make some interesting
> > progress if we could turn it into a queueing network rather than a
> > single, extremely complex queue.
> > 
> > Say we had a front-end for gerrit which tracked which queue a patch is
> > in, we could experiment with things like:
> > 
> >   - a triage queue, with non-cores signed up as triagers looking for 
> > obvious mistakes and choosing the next queue for a patch to enter 
> > into
> > 
> >   - queues having a small number of cores signed up as owners - e.g. 
> > high priority bugfix, API, scheduler, object conversion, libvirt
> > driver, vmware driver, etc.
> > 
> >   - we'd allow for a large number of queues so that cores could aim for 
> > an "inbox zero" approach on individual queues, something that would 
> > probably help keep cores motivated.
> > 
> >   - we could apply different scheduling policies to each of the 
> > different queues - i.e. explicit guidance for cores about which 
> > patches they should pick off the queue next.
> > 
> >   - we could track metrics on individual queues as well as the whole 
> > network, identifying bottlenecks and properly recognizing which 
> > reviewers are doing a small number of difficult reviews versus 
> > those doing a high number of trivial reviews.
> > 
> >   - we could requi

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Russell Bryant]

On 06/18/2014 08:31 PM, Joe Gordon wrote:
> 
> 
> 
> On Wed, Jun 18, 2014 at 5:19 PM, Clint Byrum  > wrote:
> 
> Excerpts from Duncan Thomas's message of 2014-06-17 03:56:10 -0700:
> > A far more effective way to reduce the load of trivial review issues
> > on core reviewers is for none-core reviewers to get in there first,
> > spot the problems and add a -1 - the trivial issues are then hopefully
> > fixed up before a core reviewer even looks at the patch.
> >
> > The fundamental problem with review is that there are more people
> > submitting than doing regular reviews. If you want the review queue to
> > shrink, do five reviews for every one you submit. A -1 from a
> > none-core (followed by a +1 when all the issues are fixed) is far,
> > far, far more useful in general than a +1 on a new patch.
> >
> 
> Perhaps we should incentivize having a good "reviews to patches" ratio
> somehow. There are probably quite a few people who are not ever going to
> be core reviewers, but who don't mind doing a few reviews per day.
> 
> 
> Perhaps we can add that
> to http://stackalytics.com/report/contribution/nova-group/30

I tried to add this stat once to the reviewstats [1][2] version of this
report.  Specifically, I tried to do a ratio of patches posted to
reviews *received*.  It didn't work out well, but it's worth trying
again.  I had trouble coming up with numbers that looked meaningful and
useful.  Rebasing large patch sets would skew the "patches" side.
Trivial rebase detection skews the "reviews received" side.  It was just
a mess.

[1] http://git.openstack.org/cgit/openstack-infra/reviewstats
[2] http://russellbryant.net/openstack-stats/nova-reviewers-30.txt

-- 
Russell Bryant

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Matthew Booth]

On 19/06/14 08:32, Mark McLoughlin wrote:
> Hi Armando,
> 
> On Tue, 2014-06-17 at 14:51 +0200, Armando M. wrote:
>> I wonder what the turnaround of trivial patches actually is, I bet you
>> it's very very small, and as Daniel said, the human burden is rather
>> minimal (I would be more concerned about slowing them down in the
>> gate, but I digress).
> 
>>
>> I think that introducing a two-tier level for patch approval can only
>> mitigate the problem, but I wonder if we'd need to go a lot further,
>> and rather figure out a way to borrow concepts from queueing theory so
>> that they can be applied in the context of Gerrit. For instance
>> Little's law [1] says:
>>
>> "The long-term average number of customers (in this context reviews)
>> in a stable system L is equal to the long-term average effective
>> arrival rate, λ, multiplied by the average time a customer spends in
>> the system, W; or expressed algebraically: L = λW."
>>
>> L can be used to determine the number of core reviewers that a project
>> will need at any given time, in order to meet a certain arrival rate
>> and average time spent in the queue. If the number of core reviewers
>> is a lot less than L then that core team is understaffed and will need
>> to increase.
>>
>> If we figured out how to model and measure Gerrit as a queuing system,
>> then we could improve its performance a lot more effectively; for
>> instance, this idea of privileging trivial patches over longer patches
>> has roots in a popular scheduling policy [3] for  M/G/1 queues, but
>> that does not really help aging of 'longer service time' patches and
>> does not have a preemption mechanism built-in to avoid starvation. 
>>
>> Just a crazy opinion...
>> Armando
>>
>> [1] - http://en.wikipedia.org/wiki/Little's_law
>> [2] - http://en.wikipedia.org/wiki/Shortest_job_first
>> [3] - http://en.wikipedia.org/wiki/M/G/1_queue
> 
> This isn't crazy at all. We do have a problem that surely could be
> studied and solved/improved by applying queueing theory or lessons from
> fields like lean manufacturing. Right now, we're simply applying our
> intuition and the little I've read about these sorts of problems is that
> your intuition can easily take you down the wrong path.
> 
> There's a bunch of things that occur just glancing through those
> articles:
> 
>   - Do we have an unstable system? Would it be useful to have arrival 
> and exit rate metrics to help highlight this? Over what time period 
> would those rates need to be averaged to be useful? Daily, weekly, 
> monthly, an entire release cycle?
> 
>   - What are we trying to optimize for? The length of time in the 
> queue? The number of patches waiting in the queue? The response 
> time to a new patch revision?
> 
>   - We have a single queue, with a bunch of service nodes with a wide 
> variance between their service rates, very little in the way of
> scheduling policy, a huge rate of service nodes sending jobs back 
> for rework, a cost associated with maintaining a job while it sits 
> in the queue, the tendency for some jobs to disrupt many other jobs 
> with merge conflicts ... not simple.
> 
>   - Is there any sort of natural limit in our queue size that makes the 
> system stable - e.g. do people naturally just stop submitting
> patches at some point?
> 
> My intuition on all of this lately is that we need some way to model and
> experiment with this queue, and I think we could make some interesting
> progress if we could turn it into a queueing network rather than a
> single, extremely complex queue.
> 
> Say we had a front-end for gerrit which tracked which queue a patch is
> in, we could experiment with things like:
> 
>   - a triage queue, with non-cores signed up as triagers looking for 
> obvious mistakes and choosing the next queue for a patch to enter 
> into
> 
>   - queues having a small number of cores signed up as owners - e.g. 
> high priority bugfix, API, scheduler, object conversion, libvirt
> driver, vmware driver, etc.
> 
>   - we'd allow for a large number of queues so that cores could aim for 
> an "inbox zero" approach on individual queues, something that would 
> probably help keep cores motivated.
> 
>   - we could apply different scheduling policies to each of the 
> different queues - i.e. explicit guidance for cores about which 
> patches they should pick off the queue next.
> 
>   - we could track metrics on individual queues as well as the whole 
> network, identifying bottlenecks and properly recognizing which 
> reviewers are doing a small number of difficult reviews versus 
> those doing a high number of trivial reviews.
> 
>   - we could require some queues to feed into a final approval queue 
> where some people are responsible for giving an approved patch a 
> final sanity check - i.e. there would be a class of reviewer with 
> good instincts who quickly churn through already-revi

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Mark McLoughlin]

Hi Armando,

On Tue, 2014-06-17 at 14:51 +0200, Armando M. wrote:
> I wonder what the turnaround of trivial patches actually is, I bet you
> it's very very small, and as Daniel said, the human burden is rather
> minimal (I would be more concerned about slowing them down in the
> gate, but I digress).

> 
> I think that introducing a two-tier level for patch approval can only
> mitigate the problem, but I wonder if we'd need to go a lot further,
> and rather figure out a way to borrow concepts from queueing theory so
> that they can be applied in the context of Gerrit. For instance
> Little's law [1] says:
> 
> "The long-term average number of customers (in this context reviews)
> in a stable system L is equal to the long-term average effective
> arrival rate, λ, multiplied by the average time a customer spends in
> the system, W; or expressed algebraically: L = λW."
> 
> L can be used to determine the number of core reviewers that a project
> will need at any given time, in order to meet a certain arrival rate
> and average time spent in the queue. If the number of core reviewers
> is a lot less than L then that core team is understaffed and will need
> to increase.
> 
> If we figured out how to model and measure Gerrit as a queuing system,
> then we could improve its performance a lot more effectively; for
> instance, this idea of privileging trivial patches over longer patches
> has roots in a popular scheduling policy [3] for  M/G/1 queues, but
> that does not really help aging of 'longer service time' patches and
> does not have a preemption mechanism built-in to avoid starvation. 
> 
> Just a crazy opinion...
> Armando
> 
> [1] - http://en.wikipedia.org/wiki/Little's_law
> [2] - http://en.wikipedia.org/wiki/Shortest_job_first
> [3] - http://en.wikipedia.org/wiki/M/G/1_queue

This isn't crazy at all. We do have a problem that surely could be
studied and solved/improved by applying queueing theory or lessons from
fields like lean manufacturing. Right now, we're simply applying our
intuition and the little I've read about these sorts of problems is that
your intuition can easily take you down the wrong path.

There's a bunch of things that occur just glancing through those
articles:

  - Do we have an unstable system? Would it be useful to have arrival 
and exit rate metrics to help highlight this? Over what time period 
would those rates need to be averaged to be useful? Daily, weekly, 
monthly, an entire release cycle?

  - What are we trying to optimize for? The length of time in the 
queue? The number of patches waiting in the queue? The response 
time to a new patch revision?

  - We have a single queue, with a bunch of service nodes with a wide 
variance between their service rates, very little in the way of
scheduling policy, a huge rate of service nodes sending jobs back 
for rework, a cost associated with maintaining a job while it sits 
in the queue, the tendency for some jobs to disrupt many other jobs 
with merge conflicts ... not simple.

  - Is there any sort of natural limit in our queue size that makes the 
system stable - e.g. do people naturally just stop submitting
patches at some point?

My intuition on all of this lately is that we need some way to model and
experiment with this queue, and I think we could make some interesting
progress if we could turn it into a queueing network rather than a
single, extremely complex queue.

Say we had a front-end for gerrit which tracked which queue a patch is
in, we could experiment with things like:

  - a triage queue, with non-cores signed up as triagers looking for 
obvious mistakes and choosing the next queue for a patch to enter 
into

  - queues having a small number of cores signed up as owners - e.g. 
high priority bugfix, API, scheduler, object conversion, libvirt
driver, vmware driver, etc.

  - we'd allow for a large number of queues so that cores could aim for 
an "inbox zero" approach on individual queues, something that would 
probably help keep cores motivated.

  - we could apply different scheduling policies to each of the 
different queues - i.e. explicit guidance for cores about which 
patches they should pick off the queue next.

  - we could track metrics on individual queues as well as the whole 
network, identifying bottlenecks and properly recognizing which 
reviewers are doing a small number of difficult reviews versus 
those doing a high number of trivial reviews.

  - we could require some queues to feed into a final approval queue 
where some people are responsible for giving an approved patch a 
final sanity check - i.e. there would be a class of reviewer with 
good instincts who quickly churn through already-reviewed patches 
looking for the kind of mistakes people tend to mistake when 
they're down in the weeds.

  - explicit queues for large, cross-cutting changes like coding style 
changes. Perhaps 

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Joe Gordon]

On Wed, Jun 18, 2014 at 5:19 PM, Clint Byrum  wrote:

> Excerpts from Duncan Thomas's message of 2014-06-17 03:56:10 -0700:
> > A far more effective way to reduce the load of trivial review issues
> > on core reviewers is for none-core reviewers to get in there first,
> > spot the problems and add a -1 - the trivial issues are then hopefully
> > fixed up before a core reviewer even looks at the patch.
> >
> > The fundamental problem with review is that there are more people
> > submitting than doing regular reviews. If you want the review queue to
> > shrink, do five reviews for every one you submit. A -1 from a
> > none-core (followed by a +1 when all the issues are fixed) is far,
> > far, far more useful in general than a +1 on a new patch.
> >
>
> Perhaps we should incentivize having a good "reviews to patches" ratio
> somehow. There are probably quite a few people who are not ever going to
> be core reviewers, but who don't mind doing a few reviews per day.
>
>
Perhaps we can add that to
http://stackalytics.com/report/contribution/nova-group/30


> I can think of a few ways, but one way is to make that a real statistic
> (brace yourselves for the warnings of "gaming the system") and then give
> the top 10 non-core reviews to patches ratios a shout out each release.
>
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Clint Byrum]

Excerpts from Duncan Thomas's message of 2014-06-17 03:56:10 -0700:
> A far more effective way to reduce the load of trivial review issues
> on core reviewers is for none-core reviewers to get in there first,
> spot the problems and add a -1 - the trivial issues are then hopefully
> fixed up before a core reviewer even looks at the patch.
> 
> The fundamental problem with review is that there are more people
> submitting than doing regular reviews. If you want the review queue to
> shrink, do five reviews for every one you submit. A -1 from a
> none-core (followed by a +1 when all the issues are fixed) is far,
> far, far more useful in general than a +1 on a new patch.
> 

Perhaps we should incentivize having a good "reviews to patches" ratio
somehow. There are probably quite a few people who are not ever going to
be core reviewers, but who don't mind doing a few reviews per day.

I can think of a few ways, but one way is to make that a real statistic
(brace yourselves for the warnings of "gaming the system") and then give
the top 10 non-core reviews to patches ratios a shout out each release.

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Martin Geisler]

Chris Friesen  writes:

> On 06/18/2014 08:35 AM, Duncan Thomas wrote:
>> On 18 June 2014 15:28, Matthew Booth  wrote:
>>> The answer is not always more
>>> review: there are other tools in the box. Imagine we spent 50% of the
>>> time we spend on review writing tempest tests instead.
>>
>> Or we push the work off of core into the wider community and require
>> 100% unit test coverage of every change *and* record the tempest
>> coverage of any changed lines so that the reviewer can gauge better
>> what the risks are?
>
> 100% coverage is not realistic.

I was thinking the same, but there are actually some non-trivial
projects that have 100% code coverage in their tests. These are two
large projects that I know of:

* http://www.pylonsproject.org/projects/pyramid/about: "Every release of
  Pyramid has 100% statement coverage via unit tests"

* http://www.sqlite.org/testing.html: "100% branch test coverage in an
  as-deployed configuration"

Whether 100% test coverage is worth is it another question. People
sometimes confuse "100% test coverage" with "100% bug free", which is
just wrong.

-- 
Martin Geisler

http://google.com/+MartinGeisler


pgpfLHxEQODiG.pgp
Description: PGP signature
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Chris Friesen]

On 06/18/2014 08:35 AM, Duncan Thomas wrote:

On 18 June 2014 15:28, Matthew Booth  wrote:

The answer is not always more
review: there are other tools in the box. Imagine we spent 50% of the
time we spend on review writing tempest tests instead.


Or we push the work off of core into the wider community and require
100% unit test coverage of every change *and* record the tempest
coverage of any changed lines so that the reviewer can gauge better
what the risks are?


100% coverage is not realistic.

How would you handle bugfixes that depend on specific databases?

How would you handle bugs in the unit tests themselves? (Like 1298690 
where the sqlite database used for unit tests handles regexp() 
differently than either mysql or postgres.)


Chris

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Duncan Thomas]

On 18 June 2014 15:28, Matthew Booth  wrote:
> On 18/06/14 13:31, Sean Dague wrote:
>> Even with 2 +2s you do the wrong thing. Yesterday we landed
>> baremetal tests that broke ironic. It has a ton of +1s from people
>> that have been working on those tests.
>
> This is slightly off topic, but think about that for a moment: the
> patch had a ton of peer review and 2 +2s from core reviewers, and it
> still broke. Review has significant benefits, but also a large cost,
> and it doesn't have all the answers. The answer is not always more
> review: there are other tools in the box. Imagine we spent 50% of the
> time we spend on review writing tempest tests instead.

Or we push the work off of core into the wider community and require
100% unit test coverage of every change *and* record the tempest
coverage of any changed lines so that the reviewer can gauge better
what the risks are?

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Matthew Booth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 18/06/14 13:31, Sean Dague wrote:
> On 06/18/2014 08:26 AM, Duncan Thomas wrote:
>> On 18 June 2014 10:04, Thierry Carrez 
>> wrote:
>> 
>>> As an aside, we don't really need two core reviewers to bless a
>>> trivial change: one could be considered sufficient. So a patch
>>> marked as trivial which has a number of +1s could be +2/APRVed
>>> directly by a core reviewer.
>>> 
>>> That would slightly reduce load on core reviewers, although I
>>> suspect most of the time is spent on complex patches, and
>>> trivial patches do not take that much time to process (or could
>>> even be seen as a nice break from more complex patch
>>> reviewing).
>> 
>> 
>> I think removing the need for two +2s is higher risk that you
>> think - the definition of 'trivial' gets stretched and stretched
>> over time because it allows people to get patches in
>> quicker/easier and we end up in a mess. I'm all for adding the
>> tag, but reducing the review requirements is, in my view,
>> dangerous. If a change is truly trivial then it is only going to
>> take moments for the second core to review it, so the saving
>> really is negligible compared to the risk.
> 
> Agreed.
> 
> Even with 2 +2s you do the wrong thing. Yesterday we landed
> baremetal tests that broke ironic. It has a ton of +1s from people
> that have been working on those tests.

This is slightly off topic, but think about that for a moment: the
patch had a ton of peer review and 2 +2s from core reviewers, and it
still broke. Review has significant benefits, but also a large cost,
and it doesn't have all the answers. The answer is not always more
review: there are other tools in the box. Imagine we spent 50% of the
time we spend on review writing tempest tests instead.

Matt

- -- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlOhoiYACgkQNEHqGdM8NJDdLwCffEvJqlR6ETt9BJPjrqN+0FpP
o18An25t4Z4NdRZEDIKu060h54Dd+PwR
=MpQW
-END PGP SIGNATURE-

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Sean Dague]

On 06/18/2014 08:26 AM, Duncan Thomas wrote:
> On 18 June 2014 10:04, Thierry Carrez  wrote:
> 
>> As an aside, we don't really need two core reviewers to bless a trivial
>> change: one could be considered sufficient. So a patch marked as trivial
>> which has a number of +1s could be +2/APRVed directly by a core reviewer.
>>
>> That would slightly reduce load on core reviewers, although I suspect
>> most of the time is spent on complex patches, and trivial patches do not
>> take that much time to process (or could even be seen as a nice break
>> from more complex patch reviewing).
> 
> 
> I think removing the need for two +2s is higher risk that you think -
> the definition of 'trivial' gets stretched and stretched over time
> because it allows people to get patches in quicker/easier and we end
> up in a mess. I'm all for adding the tag, but reducing the review
> requirements is, in my view, dangerous. If a change is truly trivial
> then it is only going to take moments for the second core to review
> it, so the saving really is negligible compared to the risk.

Agreed.

Even with 2 +2s you do the wrong thing. Yesterday we landed baremetal
tests that broke ironic. It has a ton of +1s from people that have been
working on those tests.

People throw +1s around with 'please do this thing', and miss the part
about 'and this current way of doing this thing is actually the correct
way to do it'.

-Sean

-- 
Sean Dague
http://dague.net



signature.asc
Description: OpenPGP digital signature
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Duncan Thomas]

On 18 June 2014 10:04, Thierry Carrez  wrote:

> As an aside, we don't really need two core reviewers to bless a trivial
> change: one could be considered sufficient. So a patch marked as trivial
> which has a number of +1s could be +2/APRVed directly by a core reviewer.
>
> That would slightly reduce load on core reviewers, although I suspect
> most of the time is spent on complex patches, and trivial patches do not
> take that much time to process (or could even be seen as a nice break
> from more complex patch reviewing).


I think removing the need for two +2s is higher risk that you think -
the definition of 'trivial' gets stretched and stretched over time
because it allows people to get patches in quicker/easier and we end
up in a mess. I'm all for adding the tag, but reducing the review
requirements is, in my view, dangerous. If a change is truly trivial
then it is only going to take moments for the second core to review
it, so the saving really is negligible compared to the risk.

-- 
Duncan Thomas

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Sean Dague]

On 06/18/2014 05:24 AM, Steven Hardy wrote:
> On Wed, Jun 18, 2014 at 11:04:15AM +0200, Thierry Carrez wrote:
>> Russell Bryant wrote:
>>> On 06/17/2014 08:20 AM, Daniel P. Berrange wrote:
 On Tue, Jun 17, 2014 at 01:12:45PM +0100, Matthew Booth wrote:
> On 17/06/14 12:36, Sean Dague wrote:
>> It could go in the commit message:
>>
>> TrivialFix
>>
>> Then could be queried with - 
>> https://review.openstack.org/#/q/message:TrivialFix,n,z
>>
>> If a reviewer felt it wasn't a trivial fix, they could just edit
>> the commit message inline to drop it out.

 Yes, that would be a workable idea.

> +1. If possible I'd update the query to filter out anything with a -1.
>
> Where do we document these things? I'd be happy to propose a docs update.

 Lets see if any other nova cores dissent, but then can add it to these 2
 wiki pages

   https://wiki.openstack.org/wiki/ReviewChecklist
   
 https://wiki.openstack.org/wiki/GitCommitMessages#Including_external_references
>>>
>>> Seems reasonable to me.
>>>
>>> Of course, I just hope it doesn't put reviewers in a mode of only
>>> looking for the trivial stuff and helping less with the big stuff.
>>
>> As an aside, we don't really need two core reviewers to bless a trivial
>> change: one could be considered sufficient. So a patch marked as trivial
>> which has a number of +1s could be +2/APRVed directly by a core reviewer.
>>
>> That would slightly reduce load on core reviewers, although I suspect
>> most of the time is spent on complex patches, and trivial patches do not
>> take that much time to process (or could even be seen as a nice break
>> from more complex patch reviewing).
> 
> Agreed, I think this actually would help improve velocity in many cases,
> provided there was sufficient common understanding of what consititutes a
> trivial patch.
> 
> The other situation this may make sense is when a non-trivial change has
> already been widely reviewed and approved then needs a last-minute minor
> rebase to resolve a simple merge conflict (e.g due to all these trivial
> patches suddenly landing really fast.. ;)
> 
> We discussed this in the Heat team a while back and agreed that having one
> core re-review and approve the patch was sufficient, and basically that
> folks could use their discretion in this situation.

Honestly, I think this is already culture in most projects. There is a
reason that we enforce 2 +2 in culture and not in code, as it allows for
fast approve with rationale. Previously approved with a merge conflict
typically falls into this category. Every group does this a little
differently, but at the end of the day the system does lean on us being
reasonable humans, which is typically a solid assumption.

-Sean

-- 
Sean Dague
http://dague.net



signature.asc
Description: OpenPGP digital signature
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Sean Dague]

On 06/18/2014 04:46 AM, Daniel P. Berrange wrote:
> On Tue, Jun 17, 2014 at 12:55:26PM -0400, Russell Bryant wrote:
>> On 06/17/2014 12:22 PM, Joe Gordon wrote:
>>>
>>>
>>>
>>> On Tue, Jun 17, 2014 at 3:56 AM, Duncan Thomas >> > wrote:
>>>
>>> A far more effective way to reduce the load of trivial review issues
>>> on core reviewers is for none-core reviewers to get in there first,
>>> spot the problems and add a -1 - the trivial issues are then hopefully
>>> fixed up before a core reviewer even looks at the patch.
>>>
>>> The fundamental problem with review is that there are more people
>>> submitting than doing regular reviews. If you want the review queue to
>>> shrink, do five reviews for every one you submit. A -1 from a
>>> none-core (followed by a +1 when all the issues are fixed) is far,
>>> far, far more useful in general than a +1 on a new patch.
>>>
>>>
>>> ++
>>>
>>> I think this thread is trying to optimize for the wrong types of
>>> patches.  We shouldn't be focusing on making trivial patches land
>>> faster, but rather more important changes such as bugs and blueprints.
>>> As some simple code motion won't directly fix any users issue such as
>>> bugs or missing features.
>>
>> In fact, landing easier and less important changes causes churn in the
>> code base can make the more important bugs and blueprints even *harder*
>> to get done.
> 
> None the less I think it is worthwhile having a way to tag trivial
> bugs so we can easily identify them. IMHO if there's a way we can
> improve turnaround time on such bugs it is worth it, if only to
> stop authors getting depressed with the wait for trivial/obvious
> fixes.

I'm definitely on that side of the fence. Actual trivial bug fix changes
(where we're talking about a couple of targeted lines) aren't very
dangerous from a merge conflict stand point.

It's the cross cutting stuff like hacking/pep8 clean ups that wreck us
on merge conflicts.

And I do agree that turn around time is important, especially for new
developers. Because you have context on an issue, and if it takes weeks
for people to get to it, by the time they do you are off to something else.

Honestly, what I really wish gerrit would do is sort based on # of lines
changed, from small to large. That would provide a good feedback loop to
make things reviewable chunks.

-Sean

-- 
Sean Dague
http://dague.net



signature.asc
Description: OpenPGP digital signature
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Steven Hardy]

On Wed, Jun 18, 2014 at 11:04:15AM +0200, Thierry Carrez wrote:
> Russell Bryant wrote:
> > On 06/17/2014 08:20 AM, Daniel P. Berrange wrote:
> >> On Tue, Jun 17, 2014 at 01:12:45PM +0100, Matthew Booth wrote:
> >>> On 17/06/14 12:36, Sean Dague wrote:
>  It could go in the commit message:
> 
>  TrivialFix
> 
>  Then could be queried with - 
>  https://review.openstack.org/#/q/message:TrivialFix,n,z
> 
>  If a reviewer felt it wasn't a trivial fix, they could just edit
>  the commit message inline to drop it out.
> >>
> >> Yes, that would be a workable idea.
> >>
> >>> +1. If possible I'd update the query to filter out anything with a -1.
> >>>
> >>> Where do we document these things? I'd be happy to propose a docs update.
> >>
> >> Lets see if any other nova cores dissent, but then can add it to these 2
> >> wiki pages
> >>
> >>   https://wiki.openstack.org/wiki/ReviewChecklist
> >>   
> >> https://wiki.openstack.org/wiki/GitCommitMessages#Including_external_references
> > 
> > Seems reasonable to me.
> > 
> > Of course, I just hope it doesn't put reviewers in a mode of only
> > looking for the trivial stuff and helping less with the big stuff.
> 
> As an aside, we don't really need two core reviewers to bless a trivial
> change: one could be considered sufficient. So a patch marked as trivial
> which has a number of +1s could be +2/APRVed directly by a core reviewer.
> 
> That would slightly reduce load on core reviewers, although I suspect
> most of the time is spent on complex patches, and trivial patches do not
> take that much time to process (or could even be seen as a nice break
> from more complex patch reviewing).

Agreed, I think this actually would help improve velocity in many cases,
provided there was sufficient common understanding of what consititutes a
trivial patch.

The other situation this may make sense is when a non-trivial change has
already been widely reviewed and approved then needs a last-minute minor
rebase to resolve a simple merge conflict (e.g due to all these trivial
patches suddenly landing really fast.. ;)

We discussed this in the Heat team a while back and agreed that having one
core re-review and approve the patch was sufficient, and basically that
folks could use their discretion in this situation.

Steve

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Thierry Carrez]

Russell Bryant wrote:
> On 06/17/2014 08:20 AM, Daniel P. Berrange wrote:
>> On Tue, Jun 17, 2014 at 01:12:45PM +0100, Matthew Booth wrote:
>>> On 17/06/14 12:36, Sean Dague wrote:
 It could go in the commit message:

 TrivialFix

 Then could be queried with - 
 https://review.openstack.org/#/q/message:TrivialFix,n,z

 If a reviewer felt it wasn't a trivial fix, they could just edit
 the commit message inline to drop it out.
>>
>> Yes, that would be a workable idea.
>>
>>> +1. If possible I'd update the query to filter out anything with a -1.
>>>
>>> Where do we document these things? I'd be happy to propose a docs update.
>>
>> Lets see if any other nova cores dissent, but then can add it to these 2
>> wiki pages
>>
>>   https://wiki.openstack.org/wiki/ReviewChecklist
>>   
>> https://wiki.openstack.org/wiki/GitCommitMessages#Including_external_references
> 
> Seems reasonable to me.
> 
> Of course, I just hope it doesn't put reviewers in a mode of only
> looking for the trivial stuff and helping less with the big stuff.

As an aside, we don't really need two core reviewers to bless a trivial
change: one could be considered sufficient. So a patch marked as trivial
which has a number of +1s could be +2/APRVed directly by a core reviewer.

That would slightly reduce load on core reviewers, although I suspect
most of the time is spent on complex patches, and trivial patches do not
take that much time to process (or could even be seen as a nice break
from more complex patch reviewing).

-- 
Thierry Carrez (ttx)

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Daniel P. Berrange]

On Tue, Jun 17, 2014 at 12:55:26PM -0400, Russell Bryant wrote:
> On 06/17/2014 12:22 PM, Joe Gordon wrote:
> > 
> > 
> > 
> > On Tue, Jun 17, 2014 at 3:56 AM, Duncan Thomas  > > wrote:
> > 
> > A far more effective way to reduce the load of trivial review issues
> > on core reviewers is for none-core reviewers to get in there first,
> > spot the problems and add a -1 - the trivial issues are then hopefully
> > fixed up before a core reviewer even looks at the patch.
> > 
> > The fundamental problem with review is that there are more people
> > submitting than doing regular reviews. If you want the review queue to
> > shrink, do five reviews for every one you submit. A -1 from a
> > none-core (followed by a +1 when all the issues are fixed) is far,
> > far, far more useful in general than a +1 on a new patch.
> > 
> > 
> > ++
> > 
> > I think this thread is trying to optimize for the wrong types of
> > patches.  We shouldn't be focusing on making trivial patches land
> > faster, but rather more important changes such as bugs and blueprints.
> > As some simple code motion won't directly fix any users issue such as
> > bugs or missing features.
> 
> In fact, landing easier and less important changes causes churn in the
> code base can make the more important bugs and blueprints even *harder*
> to get done.

None the less I think it is worthwhile having a way to tag trivial
bugs so we can easily identify them. IMHO if there's a way we can
improve turnaround time on such bugs it is worth it, if only to
stop authors getting depressed with the wait for trivial/obvious
fixes.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Matthew Booth]

On 17/06/14 17:55, Russell Bryant wrote:
> On 06/17/2014 12:22 PM, Joe Gordon wrote:
>>
>>
>>
>> On Tue, Jun 17, 2014 at 3:56 AM, Duncan Thomas > > wrote:
>>
>> A far more effective way to reduce the load of trivial review issues
>> on core reviewers is for none-core reviewers to get in there first,
>> spot the problems and add a -1 - the trivial issues are then hopefully
>> fixed up before a core reviewer even looks at the patch.
>>
>> The fundamental problem with review is that there are more people
>> submitting than doing regular reviews. If you want the review queue to
>> shrink, do five reviews for every one you submit. A -1 from a
>> none-core (followed by a +1 when all the issues are fixed) is far,
>> far, far more useful in general than a +1 on a new patch.
>>
>>
>> ++
>>
>> I think this thread is trying to optimize for the wrong types of
>> patches.  We shouldn't be focusing on making trivial patches land
>> faster, but rather more important changes such as bugs and blueprints.
>> As some simple code motion won't directly fix any users issue such as
>> bugs or missing features.
> 
> In fact, landing easier and less important changes causes churn in the
> code base can make the more important bugs and blueprints even *harder*
> to get done.

I see 3 principal advantages to getting trivial changes out of the queue
quickly:

* It reduces unnecessary rebases. The longer your code motion patch
languishes, the more times you're going to have to rebase it.

* It encourages submitters to break patches down in to a larger number
of smaller pieces. This makes it much simpler to understand and validate
a large change[1].

* It reduces frustration. It is soul destroying to have to wait weeks
for somebody to agree that you fixed a typo[2]. Unhappy developers can
be poisonous.

> In the end, as others have said, the biggest problem by far is just that
> we need more of the right people reviewing code.

Agreed, but a resource squeeze is often a good time to see
optimisations. A small improvement is still an improvement :)

Matt

[1] This series is very nice: https://review.openstack.org/#/c/98604/

[2] In fact, I'm aware of a significant amount of cleanup which hasn't
happened because of this.
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Russell Bryant]

On 06/17/2014 12:22 PM, Joe Gordon wrote:
> 
> 
> 
> On Tue, Jun 17, 2014 at 3:56 AM, Duncan Thomas  > wrote:
> 
> A far more effective way to reduce the load of trivial review issues
> on core reviewers is for none-core reviewers to get in there first,
> spot the problems and add a -1 - the trivial issues are then hopefully
> fixed up before a core reviewer even looks at the patch.
> 
> The fundamental problem with review is that there are more people
> submitting than doing regular reviews. If you want the review queue to
> shrink, do five reviews for every one you submit. A -1 from a
> none-core (followed by a +1 when all the issues are fixed) is far,
> far, far more useful in general than a +1 on a new patch.
> 
> 
> ++
> 
> I think this thread is trying to optimize for the wrong types of
> patches.  We shouldn't be focusing on making trivial patches land
> faster, but rather more important changes such as bugs and blueprints.
> As some simple code motion won't directly fix any users issue such as
> bugs or missing features.

In fact, landing easier and less important changes causes churn in the
code base can make the more important bugs and blueprints even *harder*
to get done.

In the end, as others have said, the biggest problem by far is just that
we need more of the right people reviewing code.

-- 
Russell Bryant

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Joe Gordon]

On Tue, Jun 17, 2014 at 3:56 AM, Duncan Thomas 
wrote:

> A far more effective way to reduce the load of trivial review issues
> on core reviewers is for none-core reviewers to get in there first,
> spot the problems and add a -1 - the trivial issues are then hopefully
> fixed up before a core reviewer even looks at the patch.
>
> The fundamental problem with review is that there are more people
> submitting than doing regular reviews. If you want the review queue to
> shrink, do five reviews for every one you submit. A -1 from a
> none-core (followed by a +1 when all the issues are fixed) is far,
> far, far more useful in general than a +1 on a new patch.
>

++

I think this thread is trying to optimize for the wrong types of patches.
 We shouldn't be focusing on making trivial patches land faster, but rather
more important changes such as bugs and blueprints. As some simple code
motion won't directly fix any users issue such as bugs or missing features.


>
>
>
> On 17 June 2014 11:04, Matthew Booth  wrote:
> > We all know that review can be a bottleneck for Nova patches.Not only
> > that, but a patch lingering in review, no matter how trivial, will
> > eventually accrue rebases which sap gate resources, developer time, and
> > will to live.
> >
> > It occurs to me that there are a significant class of patches which
> > simply don't require the attention of a core reviewer. Some examples:
> >
> > * Indentation cleanup/comment fixes
> > * Simple code motion
> > * File permission changes
> > * Trivial fixes which are obviously correct
> >
> > The advantage of a core reviewer is that they have experience of the
> > whole code base, and have proven their ability to make and judge core
> > changes. However, some fixes don't require this level of attention, as
> > they are self-contained and obvious to any reasonable programmer.
> >
> > Without knowing anything of the architecture of gerrit, I propose
> > something along the lines of a '+1 (trivial)' review flag. If a review
> > gained some small number of these, I suggest 2 would be reasonable, it
> > would be equivalent to a +2 from a core reviewer. The ability to set
> > this flag would be a privilege. However, the bar to gaining this
> > privilege would be low, and preferably automatically set, e.g. 5
> > accepted patches. It would be removed for abuse.
> >
> > Is this practical? Would it help?
> >
> > Matt
> > --
> > Matthew Booth
> > Red Hat Engineering, Virtualisation Team
> >
> > Phone: +442070094448 (UK)
> > GPG ID:  D33C3490
> > GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
> >
> > ___
> > OpenStack-dev mailing list
> > OpenStack-dev@lists.openstack.org
> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
>
> --
> Duncan Thomas
>
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Ben Nemec]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 06/17/2014 09:52 AM, John Griffith wrote:
> On Tue, Jun 17, 2014 at 6:51 AM, Armando M. 
> wrote:
> 
>> I wonder what the turnaround of trivial patches actually is, I
>> bet you it's very very small, and as Daniel said, the human
>> burden is rather minimal (I would be more concerned about slowing
>> them down in the gate, but I digress).
>> 
>> I think that introducing a two-tier level for patch approval can
>> only mitigate the problem, but I wonder if we'd need to go a lot
>> further, and rather figure out a way to borrow concepts from
>> queueing theory so that they can be applied in the context of
>> Gerrit. For instance Little's law [1] says:
>> 
>> "The long-term average number of customers (in this context
>> *reviews*) in a stable system L is equal to the long-term average
>> effective arrival rate, λ, multiplied by the average time a
>> customer spends in the system, W; or expressed algebraically: L =
>> λW."
>> 
>> L can be used to determine the number of core reviewers that a
>> project will need at any given time, in order to meet a certain
>> arrival rate and average time spent in the queue. If the number
>> of core reviewers is a lot less than L then that core team is
>> understaffed and will need to increase.
>> 
>> If we figured out how to model and measure Gerrit as a queuing
>> system, then we could improve its performance a lot more
>> effectively; for instance, this idea of privileging trivial
>> patches over longer patches has roots in a popular scheduling
>> policy [3] for  M/G/1 queues, but that does not really help aging
>> of 'longer service time' patches and does not have a preemption 
>> mechanism built-in to avoid starvation.
>> 
>> Just a crazy opinion... Armando
>> 
>> [1] - http://en.wikipedia.org/wiki/Little's_law [2] -
>> http://en.wikipedia.org/wiki/Shortest_job_first [3] -
>> http://en.wikipedia.org/wiki/M/G/1_queue
>> 
>> 
>> On 17 June 2014 14:12, Matthew Booth  wrote:
>> 
> On 17/06/14 12:36, Sean Dague wrote:
> On 06/17/2014 07:23 AM, Daniel P. Berrange wrote:
>> On Tue, Jun 17, 2014 at 11:04:17AM +0100, Matthew Booth
>> wrote:
>>> We all know that review can be a bottleneck for Nova 
>>> patches.Not only that, but a patch lingering in review,
>>> no matter how trivial, will eventually accrue rebases
>>> which sap gate resources, developer time, and will to
>>> live.
>>> 
>>> It occurs to me that there are a significant class of
>>> patches which simply don't require the attention of a
>>> core reviewer. Some examples:
>>> 
>>> * Indentation cleanup/comment fixes * Simple code
>>> motion * File permission changes * Trivial fixes which
>>> are obviously correct
>>> 
>>> The advantage of a core reviewer is that they have
>>> experience of the whole code base, and have proven
>>> their ability to make and judge core changes. However,
>>> some fixes don't require this level of attention, as
>>> they are self-contained and obvious to any reasonable
>>> programmer.
>>> 
>>> Without knowing anything of the architecture of gerrit,
>>> I propose something along the lines of a '+1 (trivial)'
>>> review flag. If a review gained some small number of
>>> these, I suggest 2 would be reasonable, it would be
>>> equivalent to a +2 from a core reviewer. The ability to
>>> set this flag would be a privilege. However, the bar to
>>> gaining this privilege would be low, and preferably
>>> automatically set, e.g. 5 accepted patches. It would be
>>> removed for abuse.
>>> 
>>> Is this practical? Would it help?
>> 
>> You are right that some types of fix are so
>> straightforward that most reasonable programmers can
>> validate them. At the same time though, this means that
>> they also don't really consume significant review time
>> from core reviewers.  So having non-cores' approve
>> trivial fixes wouldn't really reduce the burden on core
>> devs.
>> 
>> The main positive impact would probably be a faster turn
>> around time on getting the patches approved because it is
>> easy for the trivial fixes to drown in the noise.
>> 
>> IME any non-trivial change to gerrit is just not going to
>> happen in any reasonably useful timeframe though. Perhaps
>> an alternative strategy would be to focus on identifying
>> which the trivial fixes are. If there was an good way to
>> get a list of all pending trivial fixes, then it would
>> make it straightforward for cores to jump in and approve
>> those simple patches as a priority, to avoid them
>> languishing too long.
>> 
>> If would be nice if gerrit had simple keyword tagging so
>> any reviewer can tag an existing commit as "trivial", but
>> that doesn't seem to exist as a concept yet.
>> 
>> So an alternative perhaps submit trivial stuff using a
>> w

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[John Griffith]

On Tue, Jun 17, 2014 at 6:51 AM, Armando M.  wrote:

> I wonder what the turnaround of trivial patches actually is, I bet you
> it's very very small, and as Daniel said, the human burden is rather
> minimal (I would be more concerned about slowing them down in the gate, but
> I digress).
>
> I think that introducing a two-tier level for patch approval can only
> mitigate the problem, but I wonder if we'd need to go a lot further, and
> rather figure out a way to borrow concepts from queueing theory so that
> they can be applied in the context of Gerrit. For instance Little's law [1]
> says:
>
> "The long-term average number of customers (in this context *reviews*) in
> a stable system L is equal to the long-term average effective arrival rate,
> λ, multiplied by the average time a customer spends in the system, W; or
> expressed algebraically: L = λW."
>
> L can be used to determine the number of core reviewers that a project
> will need at any given time, in order to meet a certain arrival rate and
> average time spent in the queue. If the number of core reviewers is a lot
> less than L then that core team is understaffed and will need to increase.
>
> If we figured out how to model and measure Gerrit as a queuing system,
> then we could improve its performance a lot more effectively; for instance,
> this idea of privileging trivial patches over longer patches has roots in a
> popular scheduling policy [3] for  M/G/1 queues, but that does not really
> help aging of 'longer service time' patches and does not have a preemption
> mechanism built-in to avoid starvation.
>
> Just a crazy opinion...
> Armando
>
> [1] - http://en.wikipedia.org/wiki/Little's_law
> [2] - http://en.wikipedia.org/wiki/Shortest_job_first
> [3] - http://en.wikipedia.org/wiki/M/G/1_queue
>
>
> On 17 June 2014 14:12, Matthew Booth  wrote:
>
>> -BEGIN PGP SIGNED MESSAGE-
>> Hash: SHA1
>>
>> On 17/06/14 12:36, Sean Dague wrote:
>> > On 06/17/2014 07:23 AM, Daniel P. Berrange wrote:
>> >> On Tue, Jun 17, 2014 at 11:04:17AM +0100, Matthew Booth wrote:
>> >>> We all know that review can be a bottleneck for Nova
>> >>> patches.Not only that, but a patch lingering in review, no
>> >>> matter how trivial, will eventually accrue rebases which sap
>> >>> gate resources, developer time, and will to live.
>> >>>
>> >>> It occurs to me that there are a significant class of patches
>> >>> which simply don't require the attention of a core reviewer.
>> >>> Some examples:
>> >>>
>> >>> * Indentation cleanup/comment fixes * Simple code motion * File
>> >>> permission changes * Trivial fixes which are obviously correct
>> >>>
>> >>> The advantage of a core reviewer is that they have experience
>> >>> of the whole code base, and have proven their ability to make
>> >>> and judge core changes. However, some fixes don't require this
>> >>> level of attention, as they are self-contained and obvious to
>> >>> any reasonable programmer.
>> >>>
>> >>> Without knowing anything of the architecture of gerrit, I
>> >>> propose something along the lines of a '+1 (trivial)' review
>> >>> flag. If a review gained some small number of these, I suggest
>> >>> 2 would be reasonable, it would be equivalent to a +2 from a
>> >>> core reviewer. The ability to set this flag would be a
>> >>> privilege. However, the bar to gaining this privilege would be
>> >>> low, and preferably automatically set, e.g. 5 accepted patches.
>> >>> It would be removed for abuse.
>> >>>
>> >>> Is this practical? Would it help?
>> >>
>> >> You are right that some types of fix are so straightforward that
>> >> most reasonable programmers can validate them. At the same time
>> >> though, this means that they also don't really consume
>> >> significant review time from core reviewers.  So having
>> >> non-cores' approve trivial fixes wouldn't really reduce the
>> >> burden on core devs.
>> >>
>> >> The main positive impact would probably be a faster turn around
>> >> time on getting the patches approved because it is easy for the
>> >> trivial fixes to drown in the noise.
>> >>
>> >> IME any non-trivial change to gerrit is just not going to happen
>> >> in any reasonably useful timeframe though. Perhaps an
>> >> alternative strategy would be to focus on identifying which the
>> >> trivial fixes are. If there was an good way to get a list of all
>> >> pending trivial fixes, then it would make it straightforward for
>> >> cores to jump in and approve those simple patches as a priority,
>> >> to avoid them languishing too long.
>> >>
>> >> If would be nice if gerrit had simple keyword tagging so any
>> >> reviewer can tag an existing commit as "trivial", but that
>> >> doesn't seem to exist as a concept yet.
>> >>
>> >> So an alternative perhaps submit trivial stuff using a well
>> >> known topic eg
>> >>
>> >> # git review --topic trivial
>> >>
>> >> Then you can just query all changes in that topic to find easy
>> >> stuff to approve.
>> >
>> > It could go in the commit message:
>> >
>> > Tr

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Armando M.]

I wonder what the turnaround of trivial patches actually is, I bet you it's
very very small, and as Daniel said, the human burden is rather minimal (I
would be more concerned about slowing them down in the gate, but I digress).

I think that introducing a two-tier level for patch approval can only
mitigate the problem, but I wonder if we'd need to go a lot further, and
rather figure out a way to borrow concepts from queueing theory so that
they can be applied in the context of Gerrit. For instance Little's law [1]
says:

"The long-term average number of customers (in this context *reviews*) in a
stable system L is equal to the long-term average effective arrival rate,
λ, multiplied by the average time a customer spends in the system, W; or
expressed algebraically: L = λW."

L can be used to determine the number of core reviewers that a project will
need at any given time, in order to meet a certain arrival rate and average
time spent in the queue. If the number of core reviewers is a lot less than
L then that core team is understaffed and will need to increase.

If we figured out how to model and measure Gerrit as a queuing system, then
we could improve its performance a lot more effectively; for instance, this
idea of privileging trivial patches over longer patches has roots in a
popular scheduling policy [3] for  M/G/1 queues, but that does not really
help aging of 'longer service time' patches and does not have a preemption
mechanism built-in to avoid starvation.

Just a crazy opinion...
Armando

[1] - http://en.wikipedia.org/wiki/Little's_law
[2] - http://en.wikipedia.org/wiki/Shortest_job_first
[3] - http://en.wikipedia.org/wiki/M/G/1_queue


On 17 June 2014 14:12, Matthew Booth  wrote:

> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
> On 17/06/14 12:36, Sean Dague wrote:
> > On 06/17/2014 07:23 AM, Daniel P. Berrange wrote:
> >> On Tue, Jun 17, 2014 at 11:04:17AM +0100, Matthew Booth wrote:
> >>> We all know that review can be a bottleneck for Nova
> >>> patches.Not only that, but a patch lingering in review, no
> >>> matter how trivial, will eventually accrue rebases which sap
> >>> gate resources, developer time, and will to live.
> >>>
> >>> It occurs to me that there are a significant class of patches
> >>> which simply don't require the attention of a core reviewer.
> >>> Some examples:
> >>>
> >>> * Indentation cleanup/comment fixes * Simple code motion * File
> >>> permission changes * Trivial fixes which are obviously correct
> >>>
> >>> The advantage of a core reviewer is that they have experience
> >>> of the whole code base, and have proven their ability to make
> >>> and judge core changes. However, some fixes don't require this
> >>> level of attention, as they are self-contained and obvious to
> >>> any reasonable programmer.
> >>>
> >>> Without knowing anything of the architecture of gerrit, I
> >>> propose something along the lines of a '+1 (trivial)' review
> >>> flag. If a review gained some small number of these, I suggest
> >>> 2 would be reasonable, it would be equivalent to a +2 from a
> >>> core reviewer. The ability to set this flag would be a
> >>> privilege. However, the bar to gaining this privilege would be
> >>> low, and preferably automatically set, e.g. 5 accepted patches.
> >>> It would be removed for abuse.
> >>>
> >>> Is this practical? Would it help?
> >>
> >> You are right that some types of fix are so straightforward that
> >> most reasonable programmers can validate them. At the same time
> >> though, this means that they also don't really consume
> >> significant review time from core reviewers.  So having
> >> non-cores' approve trivial fixes wouldn't really reduce the
> >> burden on core devs.
> >>
> >> The main positive impact would probably be a faster turn around
> >> time on getting the patches approved because it is easy for the
> >> trivial fixes to drown in the noise.
> >>
> >> IME any non-trivial change to gerrit is just not going to happen
> >> in any reasonably useful timeframe though. Perhaps an
> >> alternative strategy would be to focus on identifying which the
> >> trivial fixes are. If there was an good way to get a list of all
> >> pending trivial fixes, then it would make it straightforward for
> >> cores to jump in and approve those simple patches as a priority,
> >> to avoid them languishing too long.
> >>
> >> If would be nice if gerrit had simple keyword tagging so any
> >> reviewer can tag an existing commit as "trivial", but that
> >> doesn't seem to exist as a concept yet.
> >>
> >> So an alternative perhaps submit trivial stuff using a well
> >> known topic eg
> >>
> >> # git review --topic trivial
> >>
> >> Then you can just query all changes in that topic to find easy
> >> stuff to approve.
> >
> > It could go in the commit message:
> >
> > TrivialFix
> >
> > Then could be queried with -
> > https://review.openstack.org/#/q/message:TrivialFix,n,z
> >
> > If a reviewer felt it wasn't a trivial fix, they could just edit
> > the co

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Russell Bryant]

On 06/17/2014 08:20 AM, Daniel P. Berrange wrote:
> On Tue, Jun 17, 2014 at 01:12:45PM +0100, Matthew Booth wrote:
>> -BEGIN PGP SIGNED MESSAGE-
>> Hash: SHA1
>>
>> On 17/06/14 12:36, Sean Dague wrote:
>>> On 06/17/2014 07:23 AM, Daniel P. Berrange wrote:
 If would be nice if gerrit had simple keyword tagging so any
 reviewer can tag an existing commit as "trivial", but that
 doesn't seem to exist as a concept yet.

 So an alternative perhaps submit trivial stuff using a well
 known topic eg

 # git review --topic trivial

 Then you can just query all changes in that topic to find easy
 stuff to approve.
>>>
>>> It could go in the commit message:
>>>
>>> TrivialFix
>>>
>>> Then could be queried with - 
>>> https://review.openstack.org/#/q/message:TrivialFix,n,z
>>>
>>> If a reviewer felt it wasn't a trivial fix, they could just edit
>>> the commit message inline to drop it out.
> 
> Yes, that would be a workable idea.
> 
>> +1. If possible I'd update the query to filter out anything with a -1.
>>
>> Where do we document these things? I'd be happy to propose a docs update.
> 
> Lets see if any other nova cores dissent, but then can add it to these 2
> wiki pages
> 
>   https://wiki.openstack.org/wiki/ReviewChecklist
>   
> https://wiki.openstack.org/wiki/GitCommitMessages#Including_external_references

Seems reasonable to me.

Of course, I just hope it doesn't put reviewers in a mode of only
looking for the trivial stuff and helping less with the big stuff.

-- 
Russell Bryant

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Daniel P. Berrange]

On Tue, Jun 17, 2014 at 01:12:45PM +0100, Matthew Booth wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
> 
> On 17/06/14 12:36, Sean Dague wrote:
> > On 06/17/2014 07:23 AM, Daniel P. Berrange wrote:
> >> If would be nice if gerrit had simple keyword tagging so any
> >> reviewer can tag an existing commit as "trivial", but that
> >> doesn't seem to exist as a concept yet.
> >> 
> >> So an alternative perhaps submit trivial stuff using a well
> >> known topic eg
> >> 
> >> # git review --topic trivial
> >> 
> >> Then you can just query all changes in that topic to find easy
> >> stuff to approve.
> > 
> > It could go in the commit message:
> > 
> > TrivialFix
> > 
> > Then could be queried with - 
> > https://review.openstack.org/#/q/message:TrivialFix,n,z
> > 
> > If a reviewer felt it wasn't a trivial fix, they could just edit
> > the commit message inline to drop it out.

Yes, that would be a workable idea.

> +1. If possible I'd update the query to filter out anything with a -1.
> 
> Where do we document these things? I'd be happy to propose a docs update.

Lets see if any other nova cores dissent, but then can add it to these 2
wiki pages

  https://wiki.openstack.org/wiki/ReviewChecklist
  
https://wiki.openstack.org/wiki/GitCommitMessages#Including_external_references

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Matthew Booth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 17/06/14 12:36, Sean Dague wrote:
> On 06/17/2014 07:23 AM, Daniel P. Berrange wrote:
>> On Tue, Jun 17, 2014 at 11:04:17AM +0100, Matthew Booth wrote:
>>> We all know that review can be a bottleneck for Nova
>>> patches.Not only that, but a patch lingering in review, no
>>> matter how trivial, will eventually accrue rebases which sap
>>> gate resources, developer time, and will to live.
>>> 
>>> It occurs to me that there are a significant class of patches
>>> which simply don't require the attention of a core reviewer.
>>> Some examples:
>>> 
>>> * Indentation cleanup/comment fixes * Simple code motion * File
>>> permission changes * Trivial fixes which are obviously correct
>>> 
>>> The advantage of a core reviewer is that they have experience
>>> of the whole code base, and have proven their ability to make
>>> and judge core changes. However, some fixes don't require this
>>> level of attention, as they are self-contained and obvious to
>>> any reasonable programmer.
>>> 
>>> Without knowing anything of the architecture of gerrit, I
>>> propose something along the lines of a '+1 (trivial)' review
>>> flag. If a review gained some small number of these, I suggest
>>> 2 would be reasonable, it would be equivalent to a +2 from a
>>> core reviewer. The ability to set this flag would be a
>>> privilege. However, the bar to gaining this privilege would be
>>> low, and preferably automatically set, e.g. 5 accepted patches.
>>> It would be removed for abuse.
>>> 
>>> Is this practical? Would it help?
>> 
>> You are right that some types of fix are so straightforward that 
>> most reasonable programmers can validate them. At the same time 
>> though, this means that they also don't really consume
>> significant review time from core reviewers.  So having
>> non-cores' approve trivial fixes wouldn't really reduce the
>> burden on core devs.
>> 
>> The main positive impact would probably be a faster turn around 
>> time on getting the patches approved because it is easy for the 
>> trivial fixes to drown in the noise.
>> 
>> IME any non-trivial change to gerrit is just not going to happen 
>> in any reasonably useful timeframe though. Perhaps an
>> alternative strategy would be to focus on identifying which the
>> trivial fixes are. If there was an good way to get a list of all
>> pending trivial fixes, then it would make it straightforward for
>> cores to jump in and approve those simple patches as a priority,
>> to avoid them languishing too long.
>> 
>> If would be nice if gerrit had simple keyword tagging so any
>> reviewer can tag an existing commit as "trivial", but that
>> doesn't seem to exist as a concept yet.
>> 
>> So an alternative perhaps submit trivial stuff using a well
>> known topic eg
>> 
>> # git review --topic trivial
>> 
>> Then you can just query all changes in that topic to find easy
>> stuff to approve.
> 
> It could go in the commit message:
> 
> TrivialFix
> 
> Then could be queried with - 
> https://review.openstack.org/#/q/message:TrivialFix,n,z
> 
> If a reviewer felt it wasn't a trivial fix, they could just edit
> the commit message inline to drop it out.

+1. If possible I'd update the query to filter out anything with a -1.

Where do we document these things? I'd be happy to propose a docs update.

Matt
- -- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlOgML0ACgkQNEHqGdM8NJCmpgCfbSy9bljyEqksjrJ7oRjE8LNH
8nUAoJBE5L+uAcQbew5ff/98eeqoRvW2
=+SOM
-END PGP SIGNATURE-

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Sean Dague]

On 06/17/2014 07:23 AM, Daniel P. Berrange wrote:
> On Tue, Jun 17, 2014 at 11:04:17AM +0100, Matthew Booth wrote:
>> We all know that review can be a bottleneck for Nova patches.Not only
>> that, but a patch lingering in review, no matter how trivial, will
>> eventually accrue rebases which sap gate resources, developer time, and
>> will to live.
>>
>> It occurs to me that there are a significant class of patches which
>> simply don't require the attention of a core reviewer. Some examples:
>>
>> * Indentation cleanup/comment fixes
>> * Simple code motion
>> * File permission changes
>> * Trivial fixes which are obviously correct
>>
>> The advantage of a core reviewer is that they have experience of the
>> whole code base, and have proven their ability to make and judge core
>> changes. However, some fixes don't require this level of attention, as
>> they are self-contained and obvious to any reasonable programmer.
>>
>> Without knowing anything of the architecture of gerrit, I propose
>> something along the lines of a '+1 (trivial)' review flag. If a review
>> gained some small number of these, I suggest 2 would be reasonable, it
>> would be equivalent to a +2 from a core reviewer. The ability to set
>> this flag would be a privilege. However, the bar to gaining this
>> privilege would be low, and preferably automatically set, e.g. 5
>> accepted patches. It would be removed for abuse.
>>
>> Is this practical? Would it help?
> 
> You are right that some types of fix are so straightforward that
> most reasonable programmers can validate them. At the same time
> though, this means that they also don't really consume significant
> review time from core reviewers.  So having non-cores' approve
> trivial fixes wouldn't really reduce the burden on core devs.
> 
> The main positive impact would probably be a faster turn around
> time on getting the patches approved because it is easy for the
> trivial fixes to drown in the noise.
> 
> IME any non-trivial change to gerrit is just not going to happen
> in any reasonably useful timeframe though. Perhaps an alternative
> strategy would be to focus on identifying which the trivial
> fixes are. If there was an good way to get a list of all pending
> trivial fixes, then it would make it straightforward for cores
> to jump in and approve those simple patches as a priority, to avoid
> them languishing too long.
> 
> If would be nice if gerrit had simple keyword tagging so any reviewer
> can tag an existing commit as "trivial", but that doesn't seem to
> exist as a concept yet.
> 
> So an alternative perhaps submit trivial stuff using a well known
> topic eg
> 
>   # git review --topic trivial 
> 
> Then you can just query all changes in that topic to find easy stuff
> to approve.

It could go in the commit message:

TrivialFix

Then could be queried with -
https://review.openstack.org/#/q/message:TrivialFix,n,z

If a reviewer felt it wasn't a trivial fix, they could just edit the
commit message inline to drop it out.

-Sean

-- 
Sean Dague
http://dague.net



signature.asc
Description: OpenPGP digital signature
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Gary Kotton]

On 6/17/14, 1:56 PM, "Duncan Thomas"  wrote:

>A far more effective way to reduce the load of trivial review issues
>on core reviewers is for none-core reviewers to get in there first,
>spot the problems and add a -1 - the trivial issues are then hopefully
>fixed up before a core reviewer even looks at the patch.
>
>The fundamental problem with review is that there are more people
>submitting than doing regular reviews. If you want the review queue to
>shrink, do five reviews for every one you submit.

+1

What you give is what you get!

> A -1 from a
>none-core (followed by a +1 when all the issues are fixed) is far,
>far, far more useful in general than a +1 on a new patch.
>
>
>
>On 17 June 2014 11:04, Matthew Booth  wrote:
>> We all know that review can be a bottleneck for Nova patches.Not only
>> that, but a patch lingering in review, no matter how trivial, will
>> eventually accrue rebases which sap gate resources, developer time, and
>> will to live.
>>
>> It occurs to me that there are a significant class of patches which
>> simply don't require the attention of a core reviewer. Some examples:
>>
>> * Indentation cleanup/comment fixes
>> * Simple code motion
>> * File permission changes
>> * Trivial fixes which are obviously correct
>>
>> The advantage of a core reviewer is that they have experience of the
>> whole code base, and have proven their ability to make and judge core
>> changes. However, some fixes don't require this level of attention, as
>> they are self-contained and obvious to any reasonable programmer.
>>
>> Without knowing anything of the architecture of gerrit, I propose
>> something along the lines of a '+1 (trivial)' review flag. If a review
>> gained some small number of these, I suggest 2 would be reasonable, it
>> would be equivalent to a +2 from a core reviewer. The ability to set
>> this flag would be a privilege. However, the bar to gaining this
>> privilege would be low, and preferably automatically set, e.g. 5
>> accepted patches. It would be removed for abuse.
>>
>> Is this practical? Would it help?
>>
>> Matt
>> --
>> Matthew Booth
>> Red Hat Engineering, Virtualisation Team
>>
>> Phone: +442070094448 (UK)
>> GPG ID:  D33C3490
>> GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
>>
>> ___
>> OpenStack-dev mailing list
>> OpenStack-dev@lists.openstack.org
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
>
>-- 
>Duncan Thomas
>
>___
>OpenStack-dev mailing list
>OpenStack-dev@lists.openstack.org
>http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Daniel P. Berrange]

On Tue, Jun 17, 2014 at 11:04:17AM +0100, Matthew Booth wrote:
> We all know that review can be a bottleneck for Nova patches.Not only
> that, but a patch lingering in review, no matter how trivial, will
> eventually accrue rebases which sap gate resources, developer time, and
> will to live.
> 
> It occurs to me that there are a significant class of patches which
> simply don't require the attention of a core reviewer. Some examples:
> 
> * Indentation cleanup/comment fixes
> * Simple code motion
> * File permission changes
> * Trivial fixes which are obviously correct
> 
> The advantage of a core reviewer is that they have experience of the
> whole code base, and have proven their ability to make and judge core
> changes. However, some fixes don't require this level of attention, as
> they are self-contained and obvious to any reasonable programmer.
> 
> Without knowing anything of the architecture of gerrit, I propose
> something along the lines of a '+1 (trivial)' review flag. If a review
> gained some small number of these, I suggest 2 would be reasonable, it
> would be equivalent to a +2 from a core reviewer. The ability to set
> this flag would be a privilege. However, the bar to gaining this
> privilege would be low, and preferably automatically set, e.g. 5
> accepted patches. It would be removed for abuse.
> 
> Is this practical? Would it help?

You are right that some types of fix are so straightforward that
most reasonable programmers can validate them. At the same time
though, this means that they also don't really consume significant
review time from core reviewers.  So having non-cores' approve
trivial fixes wouldn't really reduce the burden on core devs.

The main positive impact would probably be a faster turn around
time on getting the patches approved because it is easy for the
trivial fixes to drown in the noise.

IME any non-trivial change to gerrit is just not going to happen
in any reasonably useful timeframe though. Perhaps an alternative
strategy would be to focus on identifying which the trivial
fixes are. If there was an good way to get a list of all pending
trivial fixes, then it would make it straightforward for cores
to jump in and approve those simple patches as a priority, to avoid
them languishing too long.

If would be nice if gerrit had simple keyword tagging so any reviewer
can tag an existing commit as "trivial", but that doesn't seem to
exist as a concept yet.

So an alternative perhaps submit trivial stuff using a well known
topic eg

  # git review --topic trivial 

Then you can just query all changes in that topic to find easy stuff
to approve.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] A modest proposal to reduce reviewer load

[Duncan Thomas]

A far more effective way to reduce the load of trivial review issues
on core reviewers is for none-core reviewers to get in there first,
spot the problems and add a -1 - the trivial issues are then hopefully
fixed up before a core reviewer even looks at the patch.

The fundamental problem with review is that there are more people
submitting than doing regular reviews. If you want the review queue to
shrink, do five reviews for every one you submit. A -1 from a
none-core (followed by a +1 when all the issues are fixed) is far,
far, far more useful in general than a +1 on a new patch.



On 17 June 2014 11:04, Matthew Booth  wrote:
> We all know that review can be a bottleneck for Nova patches.Not only
> that, but a patch lingering in review, no matter how trivial, will
> eventually accrue rebases which sap gate resources, developer time, and
> will to live.
>
> It occurs to me that there are a significant class of patches which
> simply don't require the attention of a core reviewer. Some examples:
>
> * Indentation cleanup/comment fixes
> * Simple code motion
> * File permission changes
> * Trivial fixes which are obviously correct
>
> The advantage of a core reviewer is that they have experience of the
> whole code base, and have proven their ability to make and judge core
> changes. However, some fixes don't require this level of attention, as
> they are self-contained and obvious to any reasonable programmer.
>
> Without knowing anything of the architecture of gerrit, I propose
> something along the lines of a '+1 (trivial)' review flag. If a review
> gained some small number of these, I suggest 2 would be reasonable, it
> would be equivalent to a +2 from a core reviewer. The ability to set
> this flag would be a privilege. However, the bar to gaining this
> privilege would be low, and preferably automatically set, e.g. 5
> accepted patches. It would be removed for abuse.
>
> Is this practical? Would it help?
>
> Matt
> --
> Matthew Booth
> Red Hat Engineering, Virtualisation Team
>
> Phone: +442070094448 (UK)
> GPG ID:  D33C3490
> GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
>
> ___
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



-- 
Duncan Thomas

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

[openstack-dev] [nova] A modest proposal to reduce reviewer load

[Matthew Booth]

We all know that review can be a bottleneck for Nova patches.Not only
that, but a patch lingering in review, no matter how trivial, will
eventually accrue rebases which sap gate resources, developer time, and
will to live.

It occurs to me that there are a significant class of patches which
simply don't require the attention of a core reviewer. Some examples:

* Indentation cleanup/comment fixes
* Simple code motion
* File permission changes
* Trivial fixes which are obviously correct

The advantage of a core reviewer is that they have experience of the
whole code base, and have proven their ability to make and judge core
changes. However, some fixes don't require this level of attention, as
they are self-contained and obvious to any reasonable programmer.

Without knowing anything of the architecture of gerrit, I propose
something along the lines of a '+1 (trivial)' review flag. If a review
gained some small number of these, I suggest 2 would be reasonable, it
would be equivalent to a +2 from a core reviewer. The ability to set
this flag would be a privilege. However, the bar to gaining this
privilege would be low, and preferably automatically set, e.g. 5
accepted patches. It would be removed for abuse.

Is this practical? Would it help?

Matt
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev