Open Stack

-----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. <armamig at gmail.com>
> 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 <mbooth at redhat.com> 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
>>>>>> 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
>>> 
>>> _______________________________________________ OpenStack-dev
>>> mailing list OpenStack-dev at lists.openstack.org 
>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>>
>>
>>
>>
>>> 
_______________________________________________
>> OpenStack-dev mailing list OpenStack-dev at lists.openstack.org 
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
>> 
​If you were to use a "trivial-fix" tag as Sean mentioned I know myself
> (and likely core Nova reviewers as well) would probably be happy to
> do a quick check once a day or so and knock such changes out rather
> quickly. The tag is a bit subjective IMO so just granting a
> different process because a "submitter said so" might not always
> work out so well.

And unfortunately "trivial" fixes aren't always.  I'm thinking
specifically of a typo fix change that went into one of the Oslo libs
that accidentally changed a file in openstack/common and broke a whole
bunch of other projects.  It was the definition of trivial (a bunch of
s/a/an/ and such), but it caused a lot of grief.

Granted, everyone missed that one so cores or not didn't make a
difference, but my point is that even trivial fixes can't just be
rubber stamped.

But +1 to a tag.  Being able to quickly find simple reviews should
help avoid frustration where one of those simple reviews languishes in
the review queue for weeks.

> 
> I still agree strongly with some of the sentiment from others on
> this post however that there's a problem with some of the ratios
> here as well (contributors:reviewers and core-reviewers:reviewers).
> ​

All my +1's.

> 
> 
> 
> _______________________________________________ OpenStack-dev
> mailing list OpenStack-dev at lists.openstack.org 
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJToGOiAAoJEDehGd0Fy7uqKycH+gP7b1GuEWJ7lz8BVnNA4xLZ
Zqu5/5dumAE4VirFnhUW3SJ+D3wszLX1V8+Eo3/ikLmvKRPh0ZJWO4AzOq9p0fsk
Qv/3dbQZfaRnqYjiHLhRNDsOquoeM+KINX8YoIgk2sWkzT3iZ+/Cf2l+LEp+vM+7
YkuGzO9OnwOTrDqk8MYrNEfQloCJ9wx+Tvt5BqIqid6a7R43vr3MZyPFE3BM4Vlg
uw8/j0uj2a+Ci0cwoynh1S9OAvVCUlzRn8K9S1eVkLcvOK4v9t65OTC7p8DQqvCa
g6a58qIWVFFDdlf6l94syuU8nAUmcxZoRl25AVn5wEgZVLf7nxHdl3Xkf/5Q4yU=
=qcaA
-----END PGP SIGNATURE-----