+1 to the modified version. On Mon, Aug 15, 2011 at 2:00 PM, Arvind Prabhakar <[email protected]> wrote:
> On Sun, Aug 14, 2011 at 2:58 AM, Mohammad Nour El-Din > <[email protected]> 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 <[email protected]> > wrote: > >> On Fri, Aug 12, 2011 at 11:47 AM, Ahmed Radwan <[email protected]> > 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 <[email protected] > >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 > > >
