----- Original Message -----
From: "Alon Bar-Lev" <alonbl(a)redhat.com>
To: "Ryan Harper" <ryanh(a)us.ibm.com>
Cc: "Mark Wu" <wudxw(a)linux.vnet.ibm.com>, "Dan Kenigsberg"
<danken(a)redhat.com>, "Greg Padgett" <gpadgett(a)redhat.com>,
"Doron Fediuck" <dfediuck(a)redhat.com>, vdsm-devel(a)lists.fedorahosted.org
Sent: Monday, October 1, 2012 10:53:31 PM
Subject: Re: Change in vdsm[master]: Use 'yum clean expire-cache' instead of
'yum clean all'
----- Original Message -----
> From: "Ryan Harper" <ryanh(a)us.ibm.com>
> To: vdsm-devel(a)lists.fedorahosted.org
> Cc: "Mark Wu" <wudxw(a)linux.vnet.ibm.com>, "Dan
Kenigsberg"
> <danken(a)redhat.com>, "Greg Padgett" <gpadgett(a)redhat.com>,
> "Doron Fediuck" <dfediuck(a)redhat.com>, "Alon Bar-Lev"
> <alonbl(a)redhat.com>
> Sent: Monday, October 1, 2012 10:24:08 PM
> Subject: Re: Change in vdsm[master]: Use 'yum clean expire-cache'
> instead of 'yum clean all'
>
> * Alon Bar-Lev <alonbl(a)redhat.com> [2012-09-27 13:38]:
> > Alon Bar-Lev has posted comments on this change.
> >
> > Change subject: Use 'yum clean expire-cache' instead of 'yum
> > clean
> > all'
> > ......................................................................
> >
> >
> > Patch Set 2:
> >
> > Ok... I was discussing... I think that if you don't get +1 from
> > parties you should wait... :)
> >
> > I see -1 as final decision... for the entire change... or if
> > contributer is not cooperating.
> >
>
> I'm interested in a little clarity here.
>
> As I see it, -1 means you don't want the current version submitted.
>
> I like the idea of putting a patch on hold while various issues are
> discussed, and it seems like a -1 is the right idea here since the
> submitter can reply and original reviewer can re-review and remove
> a
> -1
> if the submitter has fully explained the issue. Additionaly the
> submitter can resubmit with changes (and the -1 is removed anyhow).
This is exactly the problem... you cannot rely on -1 as it clears if
a new patchset is pushed.
Only +1 is to be considered before merge.
> What is the maintainer view of how best to use -1? And if we don't
> use
> a -1, how does one do the above?
I think that this is fairly new to have proper procedure set.
There are people who use -1 as labelling of what they reviewed.
There are people who use -1 if they have any comment.
There are people who use -1 if they feel patch (the idea, mean and
description) should not be submitted.
My view is that among the -2, -1, 0, +1, +2 there is a proper room
for 0, 0 is a number just like any other number, removing it from
scale means narrowing the scale.
My personal view is that:
0 - discussion taking place (initial state).
+1 - approved for submission by reviewer.
+2 - approve for submission by maintainer.
-1 - either the change (principal) is rejected, or the patch is so
bad that need a major rework, or contributor is not cooperating.
-2 - final rejection by maintainer.
But this is only my view...
BTW: it works fine at engine patches... all discussion takes place at
'0' until a decision of -1 or +1 and then final decision of -2 or
+2.
Regards,
Alon.
FWIW, all marks are reset when a patch is rebased.
This does /not/ mean we can ignore patch history (unless technical issues such as
Jenkins).
As for the meaning of the numbers this is my general policy for all projects,
which I know many maintainers share. Alon's view is close in some points;
(-2) This change is either:
- Malicious, Illegal or in some other violation
- Breaks compilation, build or design
That's it. This grade is blunt. Do not use it for any other case.
(-1) Any other case where you need to block the patch.
(0) Any comment you wish to add. This may include questions and suggestions.
Use this and do not block the change if there's no *real* reason to block it.
(1) The patch works, but...
- I cannot assure it correctness system-wide.
- I'd like to reserve what I'm +1-ing for by adding a general comment (e.g.
"+1 for style, did not review correctness)
- Or "Flow looks good to me, unsure about style"
- Or "Looks correct but risky so needs second opinion" (note that in storage the
norm is to have at least 2 reviewers ack before merging).
(2) The patch works. To my best knowledge, it's correct system-wide.
And I understand the subject materia and I am willing to sign my name on this patch.
For storage- this patch already has at least 1 additional +1 about correctness, not
style.
Examples (search Gerrit for me as a reviewer);
(-2)
- Including a dependency on some package with bad licensing.
(-1)
- A patch which may break integrations with ovirt node and needs verification
(0)
- need info on something, comments for future readers, etc.
(+1)
- All vdsm patches I approve
- Engine patches related to UI / storage / networking
- Asking for improvments
(+2)
- Many infra / all SLA related patches
- All other patches which I fully understand and can vouch for.