Code review process (was Re: Code review agenda)

[Aidan Skinner <ai...@apache.org>]

On Fri, Aug 1, 2008 at 2:03 PM, Aidan Skinner <ai...@apache.org> wrote:

> I'm not sure this is really working anymore, and I'd like to discuss
> alternatives.

So, the problem I have with the way we're doing stuff just now is that
we don't do it regularly enough, and we end up with multi-hour slogs.
The pain from this becomes almost self-reinforcing, and I don't think
we're getting as high an energy/payoff ratio as we could.

So, I'd like to propose moving to an asynchronous review model for
most changes, and keep the call going but talk about the larger, more
complicated patches in more detail and ignore the more trivial ones
there.

There are two general models I can see for doing this, and a couple of
different ways between each. I have a definite preference, but I'll
just enumerate them right now.

The 20k foot view is commit-then-review (CTR), and review-then-commit
(RTC). Commit-then-review is how we've been running things up until
now.

Orthogonol to that, there's the actual review process.

Previously we'd relied on mail to qpid-commits, which is quite easy to
ignore, only works for CTR and it's hard to verify that anything has
been reviewed, let alone that everything has been reviewed. The RTC
inverse of this, the good old fashioned ask for patch approval from
the list, is easier to tell that it's been reviewed and to verify that
things have been reviewed.

Having long slogs through the commits was easier to tell that some
things had been reviewed, but we did get into a bad habit of skipping
things and skimming things in the name of time.

We could use Jira (every change has a Jira, right?), comment on that
and use the voting feature to indicate that it's been reviewed. I
can't see anything along the lines of tags so adding a
'needs_review'/'review_ok' to the issue seems out (oh Jira, why aren't
you as good as Bugzilla?)

There's specialist tooling available, such as Review Board
(http://www.review-board.org/), which is perhaps better suited than
Jira to this, but would require infrastructure (I doubt we could get
infra@ to suppport this, but I could probably help out if the load
isn't too big)

Thoughts?

- Aidan
-- 
Apache Qpid - World Domination through Advanced Message Queueing
http://cwiki.apache.org/qpid
"Nine-tenths of wisdom consists in being wise in time." - Theodore Roosevelt

Re: Code review process (was Re: Code review agenda)

[Robert Greig <ro...@gmail.com>]

2008/8/4 Alan Conway <ac...@redhat.com>:

> How/why does such a tool differ from Jira? I'd be wary of taking on more
> tools unless we can't solve the problem with the ones we have - it's
> more overhead for new contributers to learn.

I should say that I haven't used the product myself although I am
considering evaluating it for my new employer.

It's a bit more than just workflow but I think it could probably have
been implemented as a Jira plug-in, e.g. it records the comments with
the specific lines of code, provides a history etc.

For a project like Qpid it is probably not worth it but I think for
certain groups of less experienced developers it may add value.

RG

Re: Code review process (was Re: Code review agenda)

[Aidan Skinner <ai...@apache.org>]

On Mon, Aug 4, 2008 at 9:19 PM, Alan Conway <ac...@redhat.com> wrote:

> On Mon, 2008-08-04 at 20:56 +0100, Robert Greig wrote:
>> 2008/8/4 Aidan Skinner <ai...@apache.org>:
>>
>> > We could use Jira (every change has a Jira, right?), comment on that
>> > and use the voting feature to indicate that it's been reviewed. I
>> > can't see anything along the lines of tags so adding a
>> > 'needs_review'/'review_ok' to the issue seems out (oh Jira, why aren't
>> > you as good as Bugzilla?)
>>
>> Surely the way to do this with Jira (or other issue trackers) is to
>> have a new stage in the workflow which is "Awaiting Review", which
>> comes after "In Progress" and before "Resolved".
>
> +1

We'd need to get infra@ to implement custom workflow, should be doable
though (I believe Hadoop has a custom workflow).

>> Atlassian also does a tool for distributed code review that tracks
>> comments etc. - I wonder if they are willing to give an open source
>> licence as they are for Jira and Confluence?
>
> How/why does such a tool differ from Jira? I'd be wary of taking on more
> tools unless we can't solve the problem with the ones we have - it's
> more overhead for new contributers to learn.

Crucible (http://www.atlassian.com/software/crucible/) lets you
annotate patches, similar to Review Board. I think we'd find it easier
to get a custom jira workflow setup than a whole new service though.

- Aidan
-- 
Apache Qpid - World Domination through Advanced Message Queueing
http://cwiki.apache.org/qpid
"Nine-tenths of wisdom consists in being wise in time." - Theodore Roosevelt

Re: Code review process (was Re: Code review agenda)

[Alan Conway <ac...@redhat.com>]

On Mon, 2008-08-04 at 20:56 +0100, Robert Greig wrote:
> 2008/8/4 Aidan Skinner <ai...@apache.org>:
> 
> > We could use Jira (every change has a Jira, right?), comment on that
> > and use the voting feature to indicate that it's been reviewed. I
> > can't see anything along the lines of tags so adding a
> > 'needs_review'/'review_ok' to the issue seems out (oh Jira, why aren't
> > you as good as Bugzilla?)
> 
> Surely the way to do this with Jira (or other issue trackers) is to
> have a new stage in the workflow which is "Awaiting Review", which
> comes after "In Progress" and before "Resolved".

+1

> Atlassian also does a tool for distributed code review that tracks
> comments etc. - I wonder if they are willing to give an open source
> licence as they are for Jira and Confluence?

How/why does such a tool differ from Jira? I'd be wary of taking on more
tools unless we can't solve the problem with the ones we have - it's
more overhead for new contributers to learn.

Re: Code review process (was Re: Code review agenda)

[Robert Greig <ro...@gmail.com>]

2008/8/4 Aidan Skinner <ai...@apache.org>:

> We could use Jira (every change has a Jira, right?), comment on that
> and use the voting feature to indicate that it's been reviewed. I
> can't see anything along the lines of tags so adding a
> 'needs_review'/'review_ok' to the issue seems out (oh Jira, why aren't
> you as good as Bugzilla?)

Surely the way to do this with Jira (or other issue trackers) is to
have a new stage in the workflow which is "Awaiting Review", which
comes after "In Progress" and before "Resolved".

Atlassian also does a tool for distributed code review that tracks
comments etc. - I wonder if they are willing to give an open source
licence as they are for Jira and Confluence?

RG