----- Original Message -----
> From: "Doron Fediuck" <dfedi...@redhat.com>
> To: "Alon Bar-Lev" <alo...@redhat.com>
> Cc: "Mark Wu" <wu...@linux.vnet.ibm.com>, "Dan Kenigsberg" 
> <dan...@redhat.com>, "Greg Padgett" <gpadg...@redhat.com>,
> vdsm-devel@lists.fedorahosted.org, "Ryan Harper" <ry...@us.ibm.com>, "Ayal 
> Baron" <aba...@redhat.com>
> Sent: Tuesday, October 2, 2012 2:28:07 PM
> Subject: Re: Change in vdsm[master]: Use 'yum clean expire-cache' instead of 
> 'yum clean all'
> 
> ----- Original Message -----
> > From: "Alon Bar-Lev" <alo...@redhat.com>
> > To: "Ryan Harper" <ry...@us.ibm.com>
> > Cc: "Mark Wu" <wu...@linux.vnet.ibm.com>, "Dan Kenigsberg"
> > <dan...@redhat.com>, "Greg Padgett" <gpadg...@redhat.com>,
> > "Doron Fediuck" <dfedi...@redhat.com>,
> > vdsm-devel@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" <ry...@us.ibm.com>
> > > To: vdsm-devel@lists.fedorahosted.org
> > > Cc: "Mark Wu" <wu...@linux.vnet.ibm.com>, "Dan Kenigsberg"
> > > <dan...@redhat.com>, "Greg Padgett" <gpadg...@redhat.com>,
> > > "Doron Fediuck" <dfedi...@redhat.com>, "Alon Bar-Lev"
> > > <alo...@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 <alo...@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).

I agree with most but not with this one...

Keep in mind that +1 is available for all people while +2 is not.
So you cannot abuse +1 for conceptual notes.

+1 should be a statement like:
"Had I been the maintainer I would have submitted this change"
"I am not the maintainer nor do I have +2 privilege, so I here is my +1"

Of course one can note that he only capable of reviewing X among the entire 
change.
For example DBA review the database changes.
Then maintainer reintegrate the collective +1s into a complete +2.

> (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.

This would have been true if all had +2.

Thanks!

> 
> 
> 
> 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.
> 
> 
_______________________________________________
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel

Reply via email to