sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ahmed Radwan <ah...@apache.org>
Subject Re: [DISCUSS] RTC / CTR
Date Mon, 15 Aug 2011 21:09:48 GMT
+1 to the modified version.

On Mon, Aug 15, 2011 at 2:00 PM, Arvind Prabhakar <arvind@apache.org> wrote:

> On Sun, Aug 14, 2011 at 2:58 AM, Mohammad Nour El-Din
> <nour.mohammad@gmail.com> wrote:
> > OK, I see you have a lot of initial committers so the RTC can work
> > fine, I suppose at least. IMHO you can go like this:
> >
> > We should not put a global timeout, when someone is picking up the
> > patch to be reviewed whether from committers or non-committers, the
> > reviewer SHOULD do the following:
> >
> > 1- The JIRA task related to that patch is RESOLVED and then go to step
> (2).
> > 2- The committer or non-committer sends an e-mail to sqoop-dev@ asking
> > for review and the subject of the e-mail MUST be tagged with [REVIEW]
> > prefix.
> > 2- Whoever picks the patch first, replies back to the e-mail with the
> > estimated time he/she has, which is a factor you both dropped :D, to
> > review and then commit the patch.
> >  2.1- If OK, then the patch is committed, JIRA is CLOSED, then go to step
> (3)
> >  2.2- If not OK, then re-open the JIRA task and write your comments
> > and then go to step (3)
> > 3- Send a reply to the [REVIEW] thread
> >  3.1- If patch was OK, send e-mail stating that.
> >  3.2- If patch was not OK, send an e-mail with some detail about
> > where to find the comments, which should be commented on the JIRA
> > task, and hence the cycle is started from the beginning.
>
> Thanks for the proposal Mohammad. If followed, I am sure it will be
> effective. However, my concern is that it is a bit process heavy.
> Also, I feel that marking the Jira resolved without having committed
> the change is misleading.
>
> My preference would be to keep the process as informal as possible. A
> modified version of your proposal on these lines would be as follows:
>
> - The committer attaches the patch s/he would like to submit to the
> Jira and posts a code review request with at least one other committer
> identified as the reviewer.
> - If the reviewer does not respond in two days time, the committer
> sends a note to sqoop-dev asking for the reviewer to prioritize the
> review or for any other committer to volunteer for the review.
> - If the review is still not done in the next three days, the change
> may be committed and the Jira resolved/closed.
> - If the review is started, then no timeout applies and the full
> review cycle should be followed until a reasonable version of the
> change has been accepted. At which point it can be committed and the
> Jira resolved/closed.
>
> >
> >
> > *NOTE*: We can have a FishEye setup from Atlassian for better code
> > review and managing review requests as well. If that OK I can start
> > the request for Sqoop repository.
>
> I am not sure what FishEye is and how does that work with ReviewBoard.
> In the past we have extensively used ReviewBoard for review requests
> and I think most of the committers are comfortable with it. Given
> this, do you think it would benefit us in any way to move to a new
> system?
>
>
> Thanks,
> Arvind
>
> >
> > Thoughts :) ?
> >
> >
> > On Fri, Aug 12, 2011 at 9:03 PM, Arvind Prabhakar <arvind@apache.org>
> wrote:
> >> On Fri, Aug 12, 2011 at 11:47 AM, Ahmed Radwan <ahmed@apache.org>
> wrote:
> >>> Thanks Arvind,
> >>>
> >>> I prefer RTC with timeout. If we decide on timeout, what is the
> suitable
> >>> timeout period and how can we manage it for different patches (some
> patches
> >>> may require more time than others for review)? Is the timeout measured
> from
> >>> review submission or from last activity on the review? Any ideas?
> >>
> >> Good point Ahmed. I don't think there is any one scheme that will
> >> address all such concerns. Given that someone may view a change as
> >> trivial but it may be very complex for someone else, I think that the
> >> idea of a set timeout will not be fair in all cases.
> >>
> >> Another option to consider is to be like Hive project, where a
> >> committer's patch must be +1'd by another committer who actually
> >> commits it. For patches coming from non-committers, any committer who
> >> reviews it can commit it.
> >>
> >> Thanks,
> >> Arvind
> >>
> >>>
> >>> Best Regards
> >>> Ahmed
> >>>
> >>> On Fri, Aug 12, 2011 at 10:59 AM, Arvind Prabhakar <arvind@apache.org
> >wrote:
> >>>
> >>>> Hi All,
> >>>>
> >>>> We are starting to see some traction in JIRA and patch activity. I
> >>>> believe now is a good time for us to put a formal policy in place that
> >>>> guides the overall review and commit process. Different projects have
> >>>> adopted different ways of addressing this, but at a high level there
> >>>> are two - Review-Then-Commit (RTC) style, and Commit-Then-Review
> >>>> (CTR).
> >>>>
> >>>> Lets discuss this to bring out various point of views and then do a
> >>>> formal vote on the candidate policy that is acceptable to the
> >>>> majority.
> >>>>
> >>>> My thoughts: I prefer RTC with timeout provisions. Specifically, I
> >>>> feel that every change must get reviewed and if the reviewers do not
> >>>> respond within a certain time, the change can be committed.
> >>>>
> >>>> Please share your thoughts, comments and concerns on this.
> >>>>
> >>>> Thanks,
> >>>> Arvind
> >>>>
> >>>
> >>
> >
> >
> >
> > --
> > Thanks
> > - Mohammad Nour
> >   Author of (WebSphere Application Server Community Edition 2.0 User
> Guide)
> >   http://www.redbooks.ibm.com/abstracts/sg247585.html
> > - LinkedIn: http://www.linkedin.com/in/mnour
> > - Blog: http://tadabborat.blogspot.com
> > ----
> > "Life is like riding a bicycle. To keep your balance you must keep
> moving"
> > - Albert Einstein
> >
> > "Writing clean code is what you must do in order to call yourself a
> > professional. There is no reasonable excuse for doing anything less
> > than your best."
> > - Clean Code: A Handbook of Agile Software Craftsmanship
> >
> > "Stay hungry, stay foolish."
> > - Steve Jobs
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message