Dev branch "checkin" policy

[Leif Hedstrom <zw...@apache.org>]

Hi all,

I'm a little worried about the lack of checkin  and review process on 
the dev branch, that we semi-agreed on for TS development. This is 
partially my fault, I initially assumed the dev branch was going to be 
for John's cache work, but it got turned into a kitchen sink for lots of 
development. As such, I'm concerned how we are going to merge this back 
to trunk. I guess it's up to the people working on the dev branch,  but 
I see three possible alternatives:

1) Review of bugs / fixes going into dev branch as they are prepared, 
before checkins (we'd have to retrofit a few commits that have already 
happened).

2) Do a complete review of the dev branch before we merge to trunk, as 
one big "review". This would be  massive undertaking...

3) Owners of code on trunk reopens bugs, with patches, and requests 
review for migration to trunk. I.e. single bugs / commits are migrated 
to trunk as requested by the people who have committed to dev branch.


Please vote / pick one :). My +1 is for #1. In the future, I hope we can 
avoid a "dev" branch entirely, and only use branches for single, large 
feature rewrites (e.g. one branch for rewriting the HTTP SM). We'd 
still, of course, create release branches (e.g. release_2_0, 
release_2_1, etc).

Also, remember the trunk is in no way closed, yet. Don't assume that all 
fixes has to go into dev, again, it was primarily created to avoid a 
massive and potentially disruptive change to land on trunk before the 
2.0 release. I think several of the bugs committed to dev branch should 
have gone through reviews and checkins straight to the trunk.

I'm going to work on finalizing a "roadmap" for getting 2.0 for Q1, 
based on our Hackathon discussions.  I hope to have that out early next 
week, together with bug lists etc.

Cheers,

-- Leif

Re: Dev branch "checkin" policy

[Miles Libbey <mi...@yahoo-inc.com>]

Perhaps we should add a new Jira "workflow" to help highlight tickets 
that need code review.  Our process is a bit different than the standard 
Jira flow 
(http://www.atlassian.com/software/jira/docs/v3.13/default_workflow.html):
1) Ticket opened
2) Ticket Assigned to a developer
3) Ticket is worked on -- move to "in progress"
4) Submit patch, request code review
[repeat steps 3-4 as needed]
5) Code is accepted, and committed.  Ticket marked fixed.

The current Jira default workflow has 1,2,and 5.  We should be able add 
3 and 4 
(http://www.atlassian.com/software/jira/docs/v3.13/workflow.html), which 
(hopefully) would help highlight the tickets that are awaiting code review.

Miles

Leif Hedstrom may have written the following on 1/31/10 10:59 AM:
> On 01/31/2010 12:13 AM, John Plevyak wrote:
>> I started out by posting all my checkins as patches.
>>
>> I got few comments/reviews which, in the interest of progress,
>> I took as approval.
>>
>
> Yes, those were approvals.
>
>> Here is my suggestion. We need to have folks claim modules and that
>> means promise to review patches in a timely manner (say a week).
>>
>
> 100% agree. The system "fell apart" for two reasons: 1) Reviews were not
> happening in a timely manner 2) In some cases, I got the impression that
> "dev" branch was considered experimental and not under normal review
> rules. It is (or was) up to each committer to request (and hunt down)
> someone for a review. In many cases, this did happened and worked, but
> sometimes things fell through the crack.
>
> As such, I'd like to make the above a rule. If a patch has been
> proposed, and the proposer requested reviews properly, if it's not
> reviewed within two weeks (I think one week is a bit aggressive, not
> allowing for vacation etc.), an implicit approval for checkin is
> granted. We should strongly recommend that reviews happens within a week
> though, if at all possible.
>
>>
>> How about this: I will review all changes to the
>> iocore modules: eventsystem/net/aio/cache/cluster/hostdb/dns/libinktomi++,
>> and, so long as I am in town, I will do it within a week.
>>
>
> Great! We need to make a Wiki page with module owner information, we
> obviously need a bunch of people willing to review code. I think we
> should aim to have 2 reviewers per larger piece of code base, maybe not
> initially, but long term. Also, any committer should consider
> him/herself empowered to review any code.
>
> Finally, I hope no one took my email the wrong way, I'm as much (or
> more) at "fault" here as anyone, so lets work together to get the
> process to work like we want it to.
>
> Cheers!
>
> -- Leif
>

Re: Dev branch "checkin" policy

[Leif Hedstrom <zw...@apache.org>]

On 01/31/2010 12:13 AM, John Plevyak wrote:
> I started out by posting all my checkins as patches.
>
> I got few comments/reviews which, in the interest of progress,
> I took as approval.
>    

Yes, those were approvals.

> Here is my suggestion. We need to have folks claim modules and that
> means promise to review patches in a timely manner (say a week).
>    

100% agree. The system "fell apart" for two reasons: 1) Reviews were not 
happening in a timely manner 2) In some cases, I got the impression that 
"dev" branch was considered experimental and not under normal review 
rules. It is (or was) up to each committer to request (and hunt down) 
someone for a review. In many cases, this did happened and worked, but 
sometimes things fell through the crack.

As such, I'd like to make the above a rule. If a patch has been 
proposed, and the proposer requested reviews properly, if it's not 
reviewed within two weeks (I think one week is a bit aggressive, not 
allowing for vacation etc.), an implicit approval for checkin is 
granted. We should strongly recommend that reviews happens within a week 
though, if at all possible.

>
> How about this: I will review all changes to the
> iocore modules: eventsystem/net/aio/cache/cluster/hostdb/dns/libinktomi++,
> and, so long as I am in town, I will do it within a week.
>    

Great! We need to make a Wiki page with module owner information, we 
obviously need a bunch of people willing to review code. I think we 
should aim to have 2 reviewers per larger piece of code base, maybe not 
initially, but long term. Also, any committer should consider 
him/herself empowered to review any code.

Finally, I hope no one took my email the wrong way, I'm as much (or 
more) at "fault" here as anyone, so lets work together to get the 
process to work like we want it to.

Cheers!

-- Leif

Re: Dev branch "checkin" policy

[John Plevyak <jp...@acm.org>]

I started out by posting all my checkins as patches.

I got few comments/reviews which, in the interest of progress,
I took as approval.

Here is my suggestion. We need to have folks claim modules and that
means promise to review patches in a timely manner (say a week).

Having a procedure of making a jira issue
and then having a patch in jira doesn't change the essential
issue we have to have folks willing to put out the effort
to comment on the issue and review the patches otherwise it is just
overhead without any benefit.

How about this: I will review all changes to the
iocore modules: eventsystem/net/aio/cache/cluster/hostdb/dns/libinktomi++,
and, so long as I am in town, I will do it within a week.

I need some other folks to do these as well so that they can review my changes.

I suggest that we have a list somewhere (wiki?) where these the module owners are listed.

We can add a "reviewer" to a checkin and include "unavailable" if there is no review
within a week which might encourage someone to volunteer.

This is just part of the growing process for the project.  We need to get
a good eco system going which means modules have to find folks willing to take
responsibility for them.

Also, there is a good deal of cleanup and reorganization needed which is going
to require more timely review of far reaching changes in the short term than
when the codebase is better modularized.  The alternative is short term
instability.  One way of dealing with that is regressions and unit tests which
is one of the things I am working on in the cache code.

I am fine with pre or post review before lockdown of the modules I will review
because I will be reviewing all the changes eventually.  For large changes I would
prefer that they be done on a branch so that I can follow the development
and make comments as the work is progressing.   It was my thought that this
is what the dev branch would be about.  I am a bit concerned because it seems
like what Leif is saying is that nobody is reviewing the changes.   That
was not the intent.  Quite the opposite in fact.  Branches should be a safe
place to make checkins to encourage pro-active comment and permit reworking
off the trunk.

Comments?

john


On 1/30/2010 8:54 PM, Leif Hedstrom wrote:
> Hi all,
> 
> I'm a little worried about the lack of checkin  and review process on
> the dev branch, that we semi-agreed on for TS development. This is
> partially my fault, I initially assumed the dev branch was going to be
> for John's cache work, but it got turned into a kitchen sink for lots of
> development. As such, I'm concerned how we are going to merge this back
> to trunk. I guess it's up to the people working on the dev branch,  but
> I see three possible alternatives:
> 
> 1) Review of bugs / fixes going into dev branch as they are prepared,
> before checkins (we'd have to retrofit a few commits that have already
> happened).
> 
> 2) Do a complete review of the dev branch before we merge to trunk, as
> one big "review". This would be  massive undertaking...
> 
> 3) Owners of code on trunk reopens bugs, with patches, and requests
> review for migration to trunk. I.e. single bugs / commits are migrated
> to trunk as requested by the people who have committed to dev branch.
> 
> 
> Please vote / pick one :). My +1 is for #1. In the future, I hope we can
> avoid a "dev" branch entirely, and only use branches for single, large
> feature rewrites (e.g. one branch for rewriting the HTTP SM). We'd
> still, of course, create release branches (e.g. release_2_0,
> release_2_1, etc).
> 
> Also, remember the trunk is in no way closed, yet. Don't assume that all
> fixes has to go into dev, again, it was primarily created to avoid a
> massive and potentially disruptive change to land on trunk before the
> 2.0 release. I think several of the bugs committed to dev branch should
> have gone through reviews and checkins straight to the trunk.
> 
> I'm going to work on finalizing a "roadmap" for getting 2.0 for Q1,
> based on our Hackathon discussions.  I hope to have that out early next
> week, together with bug lists etc.
> 
> Cheers,
> 
> -- Leif