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