----- 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>, > firstname.lastname@example.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>, > > email@example.com > > 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: firstname.lastname@example.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 email@example.com https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel