Hi Sean,
I'm not convinced the comparison to my clean shut down change is valid here.
For sure that proved that beyond a certain point (in that case months) there is
no additional value in extending the review period, and no amount of review
will catch all problems, but that's not the same as saying that there is no
value in a minimum period.
In this particular case if the review has been open for say three weeks then
imo the issue would have been caught, as I spotted it as soon as I saw the
merge. As it wasn't and urgent bug fix I don't see a major gain from not
waiting even if there wasn't a problem.
I'm all for continually improving the gate tests, but in this case they would
need to be testing against a system that had been running before the change, to
test specifically that new vms got the new fs, so there would have needed to be
a matching test added to grenade as part of the same commit.
Not quite sure where the number of open changes comes in either, just because
there are a lot of proposed changes doesn't to me mean we need to rush the non
urgent ones, it mwans we maybe need better prioritisation. There are plenty of
long lived buf fixes siting with many +1s
Phil
Sent from Samsung Mobile
-------- Original message --------
From: Sean Dague <s...@dague.net>
Date:
To: "OpenStack Development Mailing List (not for usage questions)"
<openstack-dev@lists.openstack.org>
Subject: Re: [openstack-dev] [nova] minimum review period for functional
changes that break backwards compatibility
On 12/29/2013 03:06 AM, Day, Phil wrote:
<snip>
>> Basically, I'm not sure what problem you're trying to solve - lets tease that
>> out, and then talk about how to solve it. "Backwards incompatible change
>> landed" might be the problem - but since every reviewer knew it, having a
>> longer review period is clearly not connected to solving the problem :).
>>
>
> That assumes that a longer review period wouldn't of allowed more reviewers
> to provide input - and I'm arguing the opposite. I also think that some
> clear guidelines might have led to the core reviewers holding this up for
> longer. As I said in my original post, the intent to get more input was
> clear in the reviews, but the period wasn't IMO long enough to make sure all
> the folks who may have something to contribute could. I'd rather see some
> established guidelines than have to be constantly on the lookout for changes
> every day or so and hoping to catch them in time.
Honestly, there are currently 397 open reviews in Nova. I am not
convinced that waiting on this one would have come up with a better
decision. I'll given an alternative point of view of the graceful
shutdown patch, where we sat on that for months, had many iterations,
landed it, it added 25 minutes to all the test runs (which had been
hinted at sometime in month 2 of the review, but got lots in the mists
of time), and we had to revert it.
I'm not convinced more time brings more wisdom. We did take it to the
list, and there were no objections. I did tell Robert to wait because I
wanted to get those points of view. But they didn't show up. Because it
was holidays could we have waited longer? Sure. I'll take a bad on that
in feeling that Dec 19th wasn't really holidays yet because I was still
working. :) But, honestly, given no negative feedback on the thread in
question and no -1 on the review, the fact that folks like google
skipped ext3 entirely, means this review was probably landing regardless.
Every time we need to do a revert, we need to figure out how to catch it
the next time. "Humans be better" is really not a solution. So this
sounds like we need a guest compatibility test where we boot a ton of
different guests on each commit and make sure they all work. I'd whole
heartily support getting that in if someone wants to champion that.
That's really going to be the only way we have a systematic way of
knowing that we break SLES in the future.
So the net, we're all human, and sometimes make mistakes. I don't think
we're going to fix this with review policy changes, but we could with
actual CI enhancements.
-Sean
--
Sean Dague
Samsung Research America
s...@dague.net / sean.da...@samsung.com
http://dague.net
_______________________________________________
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
