Outstanding reviews

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

I'm concerned at the number of reviews which are outstanding, some of
which have been there for Some Time, over a month in some cases.

https://issues.apache.org/jira/secure/IssueNavigator.jspa?reset=true&&pid=12310520&status=10006&sorter/field=issuekey&sorter/order=DESC&sorter/field=updated&sorter/order=DESC

We haven't even had a particularly heavy period of change, which isn't
boding well for commit-then-review as a strategy for ensuring
everything gets reviewed.

- 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: Outstanding reviews

[Carl Trieloff <cc...@redhat.com>]

Marnie McCormack wrote:
> Hi,
>
> I think that we should discuss options here i.e. we have a defined process
> for ensuring our code quality. If this is not working then we should agree
> the outcome i.e. if you don't have an item reviewed within x weeks then the
> commit is reverted from trunk ? (For the Java items, we should not be
> releasing anything not reviewed iiuc our approach here.)
>
> Drastic I know, but it's not cricket imho to have agreed to adopt an
> approach and then have Aidan be the policeman for the project.
>
> Thoughts ?

I am not sure this will improve code quality, but rather just create 
frustration. Stuff that we are
doing now to close down the release I believe has a higher impact on 
improving code quality.

Carl.

Re: Outstanding reviews

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

On Thu, Nov 20, 2008 at 9:23 AM, Marnie McCormack
<ma...@googlemail.com> wrote:

> I'd be glad to hear any positive alternative suggestions for how we can get
> better at this stuff (without one of our number going round pinging
> everyone).
>
> This is an old difficulty - on the old Friday calls the action list didn't
> get actioned :-(

That's another point. Not only are there Jiras which have been waiting
for review for a while, there are Jiras out there that have been
reviewed, have been reopened as a result and have then not had
anything done with them for Some Time. Not all of them are reopened
(I'm sure of the open and inprogress ones are a result of that as
well), but there's at least some that that applies too.

> What can we do to make this better ? Why isn't it already working ?

I think the problem is that there is little incentive to make sure tha
tthe patch passes review once it's committed. There are probably
changes to the Jira system to make this more obvious. A 'rejected'
status rather than 'reopened' or regular summaries which are published
to the list.

I am generally not in favour of name and shame tactics like that
though. I'd rather have a carrot that encourages people to make sure
that the patch passes review, rather than a stick to beat people with
when it doesn't.

- 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: Outstanding reviews

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

On Fri, Nov 21, 2008 at 8:34 AM, Marnie McCormack
<ma...@googlemail.com> wrote:

> It's a practical idea but I'd be a little concerned about the time cost of
> this prior to release i.e. potentially reviewing months of commits ? (I know
> we've gotten far better at regular releases, but really they're still pretty
> infrequent and usually take a while in the cooking.)

I've got several concerns about this.

Reviewing code weeks or months after the person wrote it inevitably
leads to exchanges along the lines of "why is it like this?" "that's a
very good question... it's been a while since I wrote this...". On a
related note, it is much more difficult to remediate problems when
there's been a large gap between commit and review.

Big chunks of reviews lead to individual patches getting less scrutiny
in terms of both time and quality of review. If you have a big pile of
stuff to review it will be sped through and feel like a chore, which
isn't really what we want.

Ultimately though, I just don't think it'll happen. Given what's
happened so far, what I think is far more likely is that people just
won't get to it, we won't want to delay the release and they'll get,
at best, a perfunctory review.

- 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: Outstanding reviews

[Marnie McCormack <ma...@googlemail.com>]

It's a practical idea but I'd be a little concerned about the time cost of
this prior to release i.e. potentially reviewing months of commits ? (I know
we've gotten far better at regular releases, but really they're still pretty
infrequent and usually take a while in the cooking.)

What do other people think ?

It'd be good to hear from the C++-ers here too. What are you doing for code
review/testing on JIRAs ?

Bfn,
Marnie

On Thu, Nov 20, 2008 at 7:06 PM, Carl Trieloff <cc...@redhat.com>wrote:

>
>
>> What can we do to make this better ? Why isn't it already working ?
>>
>>
>>
>
> What about a review/ JIRA clean up as part of each release close down?
>
> Carl.
>

Re: Outstanding reviews

[Carl Trieloff <cc...@redhat.com>]

>
> What can we do to make this better ? Why isn't it already working ?
>
>   

What about a review/ JIRA clean up as part of each release close down?

Carl.

Re: Outstanding reviews

[Marnie McCormack <ma...@googlemail.com>]

I'd be glad to hear any positive alternative suggestions for how we can get
better at this stuff (without one of our number going round pinging
everyone).

This is an old difficulty - on the old Friday calls the action list didn't
get actioned :-(

Working together on trunk is definitely the way to go, and it does mean that
across the Java developer community we need to have a consistent approach to
the dev process. We've talked about this stuff together, on list, and agreed
how we'll do it ....

What can we do to make this better ? Why isn't it already working ?

Bfn,
Marnie

On Wed, Nov 19, 2008 at 11:08 PM, Robert Greig <ro...@gmail.com>wrote:

> 2008/11/19 Rafael Schloming <ra...@redhat.com>:
>
> > I don't think this is feasible. Commits that are on the order of weeks
> old
> > aren't trivially revertible. The work to disentangle the commit from any
> > subsequent modifications could easily be more work than simply doing the
> > review, and in many cases would itself constitute a change requiring
> review.
>
> This is a good argument for ensuring that people complete reviews promptly.
>
> RG
>

Re: Outstanding reviews

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

2008/11/19 Rafael Schloming <ra...@redhat.com>:

> I don't think this is feasible. Commits that are on the order of weeks old
> aren't trivially revertible. The work to disentangle the commit from any
> subsequent modifications could easily be more work than simply doing the
> review, and in many cases would itself constitute a change requiring review.

This is a good argument for ensuring that people complete reviews promptly.

RG

Re: Outstanding reviews

[Rafael Schloming <ra...@redhat.com>]

Marnie McCormack wrote:
> Hi,
> 
> I think that we should discuss options here i.e. we have a defined process
> for ensuring our code quality. If this is not working then we should agree
> the outcome i.e. if you don't have an item reviewed within x weeks then the
> commit is reverted from trunk ? (For the Java items, we should not be
> releasing anything not reviewed iiuc our approach here.)
> 
> Drastic I know, but it's not cricket imho to have agreed to adopt an
> approach and then have Aidan be the policeman for the project.
> 
> Thoughts ?

I don't think this is feasible. Commits that are on the order of weeks 
old aren't trivially revertible. The work to disentangle the commit from 
any subsequent modifications could easily be more work than simply doing 
the review, and in many cases would itself constitute a change requiring 
review.

--Rafael

Re: Outstanding reviews

[Marnie McCormack <ma...@googlemail.com>]

Hi,

I think that we should discuss options here i.e. we have a defined process
for ensuring our code quality. If this is not working then we should agree
the outcome i.e. if you don't have an item reviewed within x weeks then the
commit is reverted from trunk ? (For the Java items, we should not be
releasing anything not reviewed iiuc our approach here.)

Drastic I know, but it's not cricket imho to have agreed to adopt an
approach and then have Aidan be the policeman for the project.

Thoughts ?

Regards,
Marnie

On Wed, Nov 19, 2008 at 3:08 PM, Aidan Skinner <ai...@apache.org> wrote:

> I'm concerned at the number of reviews which are outstanding, some of
> which have been there for Some Time, over a month in some cases.
>
>
> https://issues.apache.org/jira/secure/IssueNavigator.jspa?reset=true&&pid=12310520&status=10006&sorter/field=issuekey&sorter/order=DESC&sorter/field=updated&sorter/order=DESC
>
> We haven't even had a particularly heavy period of change, which isn't
> boding well for commit-then-review as a strategy for ensuring
> everything gets reviewed.
>
> - 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
>