On Tue, Aug 16, 2011 at 1:13 AM, Mohammad Nour El-Din <[email protected]> wrote: > On Mon, Aug 15, 2011 at 11: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. > > I agree with this modified approach, but there is only one drawback > which is in case of a patch submitted by a non-committer, in some > situations which I saw in other projects, patches taking too much of > time to be reviewed and committed might be a negative factor for > attracting new contributors, which will put some more responsibility > on committers to take care of that.
I agree too that there is a risk that the patches submitted by non-committers may not get prioritized for review. That is not the case as of now though, and I would be very glad if our community can grow to the point where that does become a problem. I would therefore suggest that we put this modified approach to vote and then revisit this once we do start seeing a backlog of uncommitted patches. Unless anyone has some other points to discuss, I would like to call this proposal to vote tomorrow. Thanks, Arvind > >> >>> >>> >>> *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? > > I am not sure, I didn't use ReviewBoard and I used to use FishEye at > work and it was perfect. But as long as people already feel > comfortable with using ReviewBoard I don't think we need to move to > FishEye. > >> >> >> 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 >>> >> > > > > -- > Thanks > - Mohammad Nour > ---- > "Life is like riding a bicycle. To keep your balance you must keep moving" > - Albert Einstein >
