-----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. <arma...@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 <mbo...@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@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 >> >> >> 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@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----- _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev