Open Stack

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:
>
>> -----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 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.

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).​
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140617/dfd7f62a/attachment.html>