----- Original Message -----
> On 10/02/2012 04:35 PM, Alon Bar-Lev wrote:
> >
> >
> > ----- Original Message -----
> >> From: "Dan Kenigsberg" <dan...@redhat.com>
> >> To: "Alon Bar-Lev" <alo...@redhat.com>
> >> Cc: "Doron Fediuck" <dfedi...@redhat.com>, "Mark Wu"
> >> <wu...@linux.vnet.ibm.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 4:02:44 PM
> >> Subject: Re: Change in vdsm[master]: Use 'yum clean expire-cache'
> >> instead of 'yum clean all'
> >>
> >> On Tue, Oct 02, 2012 at 09:34:05AM -0400, Alon Bar-Lev wrote:
> >>>
> >>>
> >>> ----- Original Message -----
> >>>> From: "Dan Kenigsberg" <dan...@redhat.com>
> >>>> To: "Alon Bar-Lev" <alo...@redhat.com>
> >>>> Cc: "Doron Fediuck" <dfedi...@redhat.com>, "Mark Wu"
> >>>> <wu...@linux.vnet.ibm.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 3:26:31 PM
> >>>> Subject: Re: Change in vdsm[master]: Use 'yum clean
> >>>> expire-cache'
> >>>> instead of 'yum clean all'
> >>>>
> >>>> On Tue, Oct 02, 2012 at 08:59:05AM -0400, Alon Bar-Lev wrote:
> >>>>>
> >>>>>
> >>>>> ----- 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.
> >>>>
> >>>> At the moment, the job of the maintainer cannot be done by a
> >>>> script.
> >>>> The
> >>>> maintainer has to review former opinions on the patch, and check
> >>>> if
> >>>> they
> >>>> have been addressed. If a valuable reviewer gave an opinionated
> >>>> -1,
> >>>> and
> >>>> it was not addressed in a later version, the mainatainer should
> >>>> not
> >>>> take
> >>>> the patch.
> >>>>
> >>>> To me, "-1" means: "hey, Dan, please do not take this patch into
> >>>> master
> >>>> before we get an answer to my worries, unless there is a more
> >>>> urgent
> >>>> reason to take the patch earlier".
> >>>
> >>> Hi Dan,
> >>>
> >>> I don't understand why you don't treat "0" at the above...
> >>>
> >>> If there were no worries, +1 had been provided...
> >>
> >> To me, "0" means "I do not have a strong opinion, I trust other
> >> people
> >> to make the right decision, given the facts and worries that I
> >> have
> >> raised".
> >>
> >> Sometimes I do not give a +1 simply because I did not have time to
> >> review
> >> the whole code, not because I have a strong worry. "0" means "not
> >> reviewed yet" or "not endorsed yet by me but not rejected by me".
> >>
> >> A polite and well-detailed -1 should be used daily and not
> >> considered
> >> "rude".
> >>
> >> Dan.
> >>
> >
> > Yes, this is the bookmark approach...
> >
> > The problem is that most people has the -1, 0, +1 scale, and in
> > that scale -1 is actually the reject method.
> >
> > "If I was the maintainer I would have rejected this patch".
>
> we could open -2 to non-maintainers as well if it is of use.
> danken?
Personally I disagree with this approach, -2 should rarely be given and doesn't
disappear. It should only be given to people who really understand the code
properly and whose opinion is well accepted by the community.
I agree with Alon that 0 can and should be used, I do not agree with him though
that ack/nack type of discussion should take place on it though and I'll
explain why -
First of all, in gerrit there is no immediately visible difference between '0'
and no review at all so someone might have serious issues with a patch but if
she did not mark it with -1 submitter might totally miss this fact.
esp. if someone sent a new revision and the title of the cover comment for
previous version doesn't state a -1 (so maintainer doesn't know he needs to go
looking back to verify things were fixed).
This is adding overhead on maintainer now to go back to each and every review
and make sure that there are no comments that should have been addressed in 0.
Note that if someone gave a -1, normally I'd expect that person to make sure
and +1 a subsequent patch to flag to maintainer that all their problems with
the patch have been addressed.
My take on this is:
-2 - The approach taken to solve the problem is wrong and the whole thing
should either be abandoned or rewritten in a new way. I can only accept this
though if the reviewer also suggests the alternative (i.e. just saying your
code isn't good is bad form imo). e.g. stating things like 'circular
references is bad' and giving -2 but not suggesting alternatives and
explanations is bad form imo.
-1 - I think there are some issues with the current patch that should be
addressed *prior* to merging it (bugs in the code that would affect many people
etc). This would also include complex code which needs explaining (if it's too
complex for me to immediately understand then it's fine to delay merge until
either a good answer why this is mandatory is received and what the code does
or simplification of the code submitted or at worst case - comment in the code.
-1 should only be given with proper explanation, otherwise imo it's bad form.
0 - I have some *personal* style problems, questions which do not affect the
validity of the patch *or* I think there are some changes that should be made
but can definitely be done in a future patch and should not prevent merging the
current version. Note that I find this very important to actually improve our
current way of working. This means that if a patch improves current code but
could in itself be further improved, it is valid imo to accept current version
and ask committer to submit another patch to further improve it.
Note that this would include things like (e.g.) discussions about spacing which
are not enforced by pep8 tools (i.e. preventing a patch which fixes bugs from
going in because of personal interpretation of pep8 about alignment of
parameters in function signature is wrong imo).
+1 - I have reviewed the code and it looks correct to me but I'm not a subject
matter expert / the maintainer.
Note that for things like style review only '+1' should be accompanied by
a cover commit message stating - "+1 only for style" as Doron has mentioned
previously on this thred.
+2 - I am a subject matter expert, I have reviewed the code and it looks good
to me (solves the problem properly and no serious issues left with it).
As Doron mentioned, in our group (storage) the standard is to have at least 2
reviews (by different people) before committing unless the patch is *really*
trivial.
This means that I try to avoid giving +2 if no else has given a +1 before.
Alon, note that we apply this both to vdsm and engine.
> _______________________________________________
> vdsm-devel mailing list
> vdsm-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
>
_______________________________________________
vdsm-devel mailing list
vdsm-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
