sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Arvind Prabhakar <arv...@apache.org>
Subject Re: [DISCUSS] RTC / CTR
Date Tue, 16 Aug 2011 17:03:59 GMT
On Tue, Aug 16, 2011 at 1:13 AM, Mohammad Nour El-Din
<nour.mohammad@gmail.com> wrote:
> On Mon, Aug 15, 2011 at 11: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.
>
> 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 <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
>>>
>>
>
>
>
> --
> Thanks
> - Mohammad Nour
> ----
> "Life is like riding a bicycle. To keep your balance you must keep moving"
> - Albert Einstein
>

Mime
View raw message