Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> George Dunlap <george.dunlap@xxxxxxxxxx> 04/15/16 1:23 PM >>>
>On Thu, Apr 14, 2016 at 6:01 PM, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>Sure, mistakes happen; but I hope it's not being to controversial to
>>>say that in general, the procedure should be arranged such that the
>>>person who makes the mistake is the one who has to do deal with the
>>>most consequences from the mistake, not the people around him.  I
>>>mean, if you asked a bartender for a bottle of beer, and after he'd
>>>already opened it you said, "Oh sorry, I actually wanted a cider", it
>>>would be fair enough for the bartender to ask you to pay for the beer,
>>>rather than eating it*, wouldn't it? :-)
>>
>> Sure. And I think that's what I've done.

To be fair, I think this was too broad a statement: I had indeed asked Konrad
to collect the replacement ack. Yet on second thought I really can't see
what's wrong with withdraing an ack - just like a patch can be withdrawn, that
should be possible for an ack too. And if the patch has already gone in, no
matter whether the patch itself or the ack got withdrawn, the patch imo should
then be reverted. I agree that's not something spelled out anywhere, so my
opinion here is based on solely general considerations of mine.

>When Konrad asked for further input on the patch and then didn't get
>any in a few days, you responded, "It looks like it will have to be
>reverted then." As I've said, I think that's the wrong way round --
>it's not that the commit is reverted unless someone else acks it; it's
>that the commit stays unless someone acks your reversion.  If you had
>posted a patch (probably RFC) requesting to revert the change in favor
>of a different one, then it would have been more obvious that the
>burden was on you to get the reversion Acked, rather than on Konrad to
>get the existing commit re-acked.

I agree on the process aspect (albeit among the few cases where things
needed reverting, I think there was a non-negligible amount of such where
the revert was requested quite informally), but that's relatively moot with me
not agreeing on the premises it builds upon.

>>>Well Ian and I have already given our opinions -- Ian thinks moving to
>>>a clean interface and deprecating the old one is in general a good
>>>thing, and doesn't look too painful in this case.  I don't really see
>>>a problem with using a large fixed size, but I don't fundamentally see
>>>a problem with moving to a new clean interface either.  Given that
>>>Andy has a strong aversion to the way things are, if you had only a
>>>mild distaste rather than a  strong objection to the new hypercall, it
>>>would probably make sense to go with the new hypercall.
>>>
>>>If you do have a strong objection, then maybe we could try to convince
>>>Andy to accept adding subops with different calling semantics to the
>>>existing hypercall.  But I would still think the burden of persuasion
>>>is primarily on you.
>>
>>  I do not have a strong objection, or else I would have converted my ack
>> into a nak instead of just withdrawing it. I just dislike the duplication, 
>> and
>> hence I'm not happy with me now being the one having approved it to go
>> in. Hence the request for a replacement ack (or whatever else referee
>> decision).
>
>Well this makes it sound like you're saying that you were asking us to
>save you from having to appear to have approved of a patch that you
>didn't like.  Which doesn't sound very nice. :-)

I'm sorry if it's being understood that way. My intention really was to avoid
the revert in case a replacement ack can be obtained. Otherwise, following
my way of thinking outlined above, the patch should have been reverted
right away. Yet I seem to be the only one here thinking this way...

>But I wonder if something slightly different is going on.  Forgive me
>for trying to guess at motivations here, but I think it may be
>helpful.  I'm often in the situation where my gut objects to a patch
>that other coders I respect think is fine; and often a few years down
>the road, I look back and agree that it's fine as well.  In other
>words, I know that sometimes my own objections turn out to be
>unreasonable; and that in any case, working with other people you
>sometimes have to compromise.  But on the other hand, I've also had
>the experience of giving in and accepting patches that later I regret,
>or that other people come back and say, "This was a terrible idea, you
>should have stood up for it more."
>
>So in the moment -- particularly, as you say, under time pressure --
>how do you determine if objecting to the patch is being unreasonable
>and obstructive, or if assenting to the patch is failing your duty as
>a maintainer to prevent bad code, and is a decision you'll regret
>later?
>
>Was it perhaps actually the case that your internal reasoning was
>along the lines of, "Actually, adding a new hypercall seems like a bad
>idea.  But since Andrew strongly disagrees, maybe I'm being
>unreasonable.  Or, maybe it really is a bad idea and he's being
>unreasonable.  Why don't I ask the REST maintainers to look at it; if
>they Ack it, then I'm probably being too conservative; if they don't,
>then I'm probably justified in objecting to it"?

Well, not really. My main problem here (and not just here) is that often
I end up reviewing patches for being done correctly, but ignore the question
of "Do we need this in the first place?" This has happened to me more
often earlier on, but it continues to happen from time to time, like on this
occasion.

>If so, we should probably find a better way for maintainers to ask for
>additional feedback in those situations. :-)  I'd certainly appreciate
>that at times.

Well, the main problem I'm seeing here that the current set of REST
maintainers makes it very hard to get _any_ kind of feedback without
explicitly asking and pinging for it. I sincerely hope that this will change
with the pending committership and maintainership adjustments. My
general understanding here is that it shouldn't be really necessary to
explicitly ask for a second opinion - after all maintainers get Cc-ed on
patches for the reason of seeking their input. And the question of
replacing an existing public interface with a slightly different new one,
even more so with no obvious route of deprecating the old one, is
something that any one of the REST maintainers should really have
an opinion on imo.

>If that's not the case, and you genuinely only have a mild dislike for
>the hypercall, then there are a couple of things to say about that.
>The first is that given the circumstances, it seems to me that giving
>the Ack was not only reasonable, but the right thing to do.  An Ack
>doesn't mean, "I think this is a good idea", but it means "Given all
>considerations, I think this should be in the tree."  And given that
>Andrew had strong objections to the other solutions, and you only had
>a distaste for this one, this is obviously the solution to choose.
>Acking a patch on the basis that you don't really like it but it's the
>best compromise with the other maintainers is perfectly reasonable.

Taking a strictly formal perspective, there was no disagreement between
maintainers, since I've been the only one of those currently named under
REST involved in the discussion. Which is part of the problem, as said
above. And Andrew's strong objection was, in my view, based on not very
convincing arguments. Hence I couldn't really see myself making a
reasonable compromise here.

>Secondly,  I can certainly see that you might be a bit embarrased
>about having your name on a patch you dislike, but... well, you did in
>fact approve it going in. :-)  Maybe that was because you were in
>haste, and you regret it, but that's an accurate record of what
>happened.  Stand up and take responsibility for your actions. :-)

Which is why - see above - I didn't revert it right away.

>> And btw., considering that Konrad has already posted a revert patch,
>> and I have ack-ed that one, this could now go in right away (and the
>> discussion could either be settled or start over).
>
>Yes, unless someone objects to that patch then we have a clear way
>forward, and we're just discussing principles for future reference at
>this point.

Agreed. As said in reply to Ian - perhaps worth taking some time at the
hackathon.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel