You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Aidan Skinner <ai...@apache.org> on 2008/08/14 16:16:15 UTC
Jira workflow change ticket and code review process.
I've raised an INFRA ticket to get a 'Ready for review' status in
between 'In Progress' and 'Resolved'.
I'd quite like to move the Java client and broker to a
review-then-commit model, with any one committer other than the author
being sufficent.
- Aidan (who is donning his asbestos suit)
--
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: Jira workflow change ticket and code review process.
Posted by Aidan Skinner <ai...@apache.org>.
On Thu, Aug 14, 2008 at 4:22 PM, Andrew Stitcher <as...@redhat.com> wrote:
> I do agree that code is always better when it's reviewed, but I think
> anything like this would have to be enforced (at least somewhat) for
> this to have value.
That's why I'm attempting to enforce it. :)
> Also attached patches start to rot after a while and without info as to
> what version they change they can be hard to apply later.
I totally agree this is a problem, I just don't think it's a big
enough problem that it makes review-then-commit impossible. People did
it before we had fancy schmancy tooling. ;)
>> [1] For $DVCS == git, as it is clearly superior to bzr and hg in crucial ways
>
> Care to elaborate? I've only used git, so have no real idea of the
> differences.
hg causes a merge that needs resolved by hand every time, even if
there aren't any conflicts, so rebasing is more expensive (it works
okish for patch based workflows like Mozilla has).
bzr branches are basically complete copies, so are more expensive in
terms of disk space and it also lacks rebase (but does have loom for
quilt style things, and a rebase plugin, but it's not as well
integrated as git).
- 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: Jira workflow change ticket and code review process.
Posted by Robert Godfrey <ro...@gmail.com>.
> I do agree that code is always better when it's reviewed, but I think
> anything like this would have to be enforced (at least somewhat) for
> this to have value.
>
>From the Java side we have been attempting to put in place reviews
before code is considered eligible for release. Up until now we have
been doing this in a weekly review call... As the volume of change
increases the feasibility of this decreases - so moving to a
distributed review model works better. certainly at release time I
would expect us to review all JIRAs to make sure their code was
reviewed. Also I would be reviewing all code committed without a JIRA
and asking for a JIRA to be created explaining the change... however
that is purely on the Java side... I don't believe there has been a
similar process on the C++ side of the house...
-- Rob
Re: Jira workflow change ticket and code review process.
Posted by Andrew Stitcher <as...@redhat.com>.
On Thu, 2008-08-14 at 15:50 +0100, Aidan Skinner wrote:
> On Thu, Aug 14, 2008 at 3:21 PM, Andrew Stitcher <as...@redhat.com> wrote:
>
> > On Thu, 2008-08-14 at 15:16 +0100, Aidan Skinner wrote:
> >> I've raised an INFRA ticket to get a 'Ready for review' status in
> >> between 'In Progress' and 'Resolved'.
> >>
> >> I'd quite like to move the Java client and broker to a
> >> review-then-commit model, with any one committer other than the author
> >> being sufficent.
> >
> > 1. I assume this is intended for changes to released code.
>
> I was kinda thinking for all the Java stack actually. We don't do much
> that is entirely new development.
I do agree that code is always better when it's reviewed, but I think
anything like this would have to be enforced (at least somewhat) for
this to have value.
>
> > 2. Without actually commiting the change the reviewer doesn't easily get
> > to see the change, and no I don't consider attached patches to be
> > adequate with svn.
>
> Why not? Is it just the rebasing problem?
Applying patches, and tracking them is just not well supported by svn.
It works well for git, but we can hardly require a reviewer to run a
different vcs just to do their task.
For example I've been reviewing the Windows and Solaris patches, but
doing it from the bare patch isn't adequate as you can't be sure it will
apply etc.
Also attached patches start to rot after a while and without info as to
what version they change they can be hard to apply later.
>
> Personally I'd prefer small branches that can be reviewed then merged,
> but I don't think that's realistic until and unless we move to
> $DVCS[1].
>
> - Aidan
>
> [1] For $DVCS == git, as it is clearly superior to bzr and hg in crucial ways
Care to elaborate? I've only used git, so have no real idea of the
differences.
Andrew
Re: Jira workflow change ticket and code review process.
Posted by Alan Conway <ac...@redhat.com>.
On Tue, 2008-08-19 at 08:53 +0200, Arnaud Simon wrote:
> On Mon, 2008-08-18 at 23:06 +0100, Robert Greig wrote:
> > 2008/8/18 Aidan Skinner <ai...@apache.org>:
> >
> > > That's why having a pool is better, if one person is busy with other
> > > stuff then others can pick up the load. If everybody is crunching like
> > > a maniac, it's probably a sign that careful review is even more
> > > necessary - I know the quality of my patches has an inverse
> > > relationship to the amount of Irn Bru consumed. ;)
> >
> > I don't like the idea of a pool basically because of the lack of accountability.
> >
> > > I don't think there's a significant difference in the toolset between
> > > commit-then-review and review-then-commit, and certainly not a
> > > compelling one that outweighs the other advantages of reviewing first.
> >
> > OK, FWIW here's what I would do. No review board, just:
> >
> > 1) work on issue and commit changes
> > 2) mail or IM someone asking if he would mind reviewing it
> > 3) assign issue to person doing review and move on until issue is
> > either reopened by reviewer or resolved (or whatever state comes next)
> > 4) run jira report regularly of issues in "pending review" state with
> > no activity for 5 days. For those issues go back to step (2) with
> > alternative reviewer if necessary.
> >
> > I think it would be useful to get some of the other committers' views
> > on what would work for them.
> >
> > RG
>
> +1 to this suggestion
>
Big +1 from me.
Speaking from my current immersion in cluster performance issues, having
2 asynchronous streams of activity that don't block each other is the
Only Way, every time you put a synchronous bubble in the workflow
performance goes straight down the toilet. It's driving me crazy.
Re: Jira workflow change ticket and code review process.
Posted by Arnaud Simon <as...@redhat.com>.
On Mon, 2008-08-18 at 23:06 +0100, Robert Greig wrote:
> 2008/8/18 Aidan Skinner <ai...@apache.org>:
>
> > That's why having a pool is better, if one person is busy with other
> > stuff then others can pick up the load. If everybody is crunching like
> > a maniac, it's probably a sign that careful review is even more
> > necessary - I know the quality of my patches has an inverse
> > relationship to the amount of Irn Bru consumed. ;)
>
> I don't like the idea of a pool basically because of the lack of accountability.
>
> > I don't think there's a significant difference in the toolset between
> > commit-then-review and review-then-commit, and certainly not a
> > compelling one that outweighs the other advantages of reviewing first.
>
> OK, FWIW here's what I would do. No review board, just:
>
> 1) work on issue and commit changes
> 2) mail or IM someone asking if he would mind reviewing it
> 3) assign issue to person doing review and move on until issue is
> either reopened by reviewer or resolved (or whatever state comes next)
> 4) run jira report regularly of issues in "pending review" state with
> no activity for 5 days. For those issues go back to step (2) with
> alternative reviewer if necessary.
>
> I think it would be useful to get some of the other committers' views
> on what would work for them.
>
> RG
+1 to this suggestion
Re: Jira workflow change ticket and code review process.
Posted by Robert Godfrey <ro...@gmail.com>.
2008/8/18 Robert Greig <ro...@gmail.com>:
> 2008/8/18 Aidan Skinner <ai...@apache.org>:
>
>> That's why having a pool is better, if one person is busy with other
>> stuff then others can pick up the load. If everybody is crunching like
>> a maniac, it's probably a sign that careful review is even more
>> necessary - I know the quality of my patches has an inverse
>> relationship to the amount of Irn Bru consumed. ;)
>
> I don't like the idea of a pool basically because of the lack of accountability.
>
>> I don't think there's a significant difference in the toolset between
>> commit-then-review and review-then-commit, and certainly not a
>> compelling one that outweighs the other advantages of reviewing first.
>
> OK, FWIW here's what I would do. No review board, just:
>
> 1) work on issue and commit changes
> 2) mail or IM someone asking if he would mind reviewing it
> 3) assign issue to person doing review and move on until issue is
> either reopened by reviewer or resolved (or whatever state comes next)
> 4) run jira report regularly of issues in "pending review" state with
> no activity for 5 days. For those issues go back to step (2) with
> alternative reviewer if necessary.
>
> I think it would be useful to get some of the other committers' views
> on what would work for them.
I agree with Robert's proposed process... so +1 to them
I afraid I really can't see how any review-then-commit process will work.
Firstly the code that needs to be reviewed is the code that is
actually committed to the repository... patches in the JIRA may or may
not represent what actually gets committed particularly if merging
with other changes has to occur... the only way this can be verified
is if you re-review after commit.... which means you need to do a
review-commit-review process... which seems a little review heavy....
Secondly I still haven't got my head round who would be responsible
for merging changes with a changed baseline... If a reviewer picks up
a patch and it doesn't apply cleanly does (s)he go back to the
original author of the change... Even if it appears to apply cleanly
perhaps other parts of the code have changed underneath it...
presumably the original author has to run the unit tests again after a
successful review but before commit to guard against this... Frankly
unless the developer and reviewer are physically co-located I don't
see how this process can work without significant asynchronous lag
between the steps... Which given the relatively small size of Qpid
and the relatively few number of hot-spots where code is concentrated
(certainly on the Java side) means that I can see patches getting lost
in an infinite loop of reviewing, small change due to merge, review,
small change due to merge... etc
So... -1 to review-then-commit
-- Rob
Re: Jira workflow change ticket and code review process.
Posted by Martin Ritchie <ri...@apache.org>.
2008/8/18 Steve Huston <sh...@riverace.com>:
>> -----Original Message-----
>> From: Ted Ross [mailto:tross@redhat.com]
>>
>> Robert Greig wrote:
>> > OK, FWIW here's what I would do. No review board, just:
>> >
>> > 1) work on issue and commit changes
>> > 2) mail or IM someone asking if he would mind reviewing it
>> > 3) assign issue to person doing review and move on until issue is
>> > either reopened by reviewer or resolved (or whatever state
>> comes next)
>> > 4) run jira report regularly of issues in "pending review"
>> state with
>> > no activity for 5 days. For those issues go back to step (2) with
>> > alternative reviewer if necessary.
>> >
>> > I think it would be useful to get some of the other
>> committers' views
>> > on what would work for them.
>> >
>> > RG
>> +1 to Robert's suggestion
>>
>> I think commit-then-review will be more efficient given the
>> capabilities of our tools.
>
> I agree with this as well, modulo two points. These may be covered
> already, but I haven't seen evidence of it...
>
> 1. If this process gets too far between commit and review, there's
> increased chance there will be changes add in after the commit that
> may confuse the reviewer.
>
> 2. I haven't seen any sort of log or diary of changes. Something that
> would, for example, list the files changed and why. Is this normally
> put in the svn commit comments? Explanatory entries in such a place
> could help the reviewer get his/her bearings and help understand what
> the committer was trying to do. Or is this info expected to be in
> jira?
>
> Also, is there someplace relatively easy to check to see if a
> particular commit broke something?
>
> Thanks,
> -Steve
Here are my thoughts on this issue.
We need to have something lightweight that actually gets done. We've
been at Apache nearly two years and we still don't update JIRAs or
provide test details (or even the actual test) for our changes.
I'd like to think it was the lack of a documented process but I don't
know if that would help.
Both the suggestions here have merits but both also ask us to do more
than we currently do and for that reason I'm not sure how successful
either will be.
We can all agree that review and testing are important but unless we
can show commitment to those goals then I fear that we are going to
continually face large code review session with growing action log.
We have previously discussed having component leads and I think that
is where we should start here. A few people that are committed to the
Qpid project and to the quality of the code that it produces.
I would say that we should have a group (2 or 3 people) who are
responsible for reviewing incoming changes to a particular component.
This should prevent any one person being a blocking in the process.
The group should be formed from those that want to dedicate their time
to our goals of review and testing. These review groups should
document the 'quality bar' that they are checking for so that existing
commiters and external patch providers can understand the exit gate of
the review process.
I still have concerns that changes such as the addition of tests or
documentation will be hard to chase up.
We need to first of all agree what we are trying to get out of this
process change. Are we looking for more control over the committed
code? Do we want to ensure we have more thoroughly reviewed code and
improved web documentation?
Once we can agree on the goals I think it will be easier to look at
the path to get there.
Regards
--
Martin Ritchie
Re: Jira workflow change ticket and code review process.
Posted by Aidan Skinner <ai...@apache.org>.
On Tue, Aug 19, 2008 at 10:37 AM, Robert Godfrey
<ro...@gmail.com> wrote:
> I think if we want to have a formal commit-then-review process then
> linking each commit to a JIRA will be necessary so that we can
> document the review.
That would be ideal, yes...
- 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: Jira workflow change ticket and code review process.
Posted by Gordon Sim <gs...@redhat.com>.
Robert Godfrey wrote:
>> It sounds like consensus is building around commit-then-review with
>> Jira, so let's go for that. I'll start a formal vote later today.
>>
>
> One thing I would like to see is some discussion about whether we will
> all be following a common process or whether the components in each
> language will be doing their own thing.
>
> The process and meetings up until now have focused mainly on the Java
> side of things; and we have been quite keen on enforcing things like
> "no commit without an accompanying JIRA"...
>
> I think if we want to have a formal commit-then-review process then
> linking each commit to a JIRA will be necessary so that we can
> document the review.
I already informally review quite a lot of commits. I focus more on
those that touch areas of the code I am familiar with and the depth of
review tends to vary with the amount of time I have and the nature of
the revision.
When I make changes that I feel may particularly warrant review for
whatever reason, I will often send out a patch before committing it
either to the list or to someone who I know to be familiar with the area
in question.
I fully accept that the code doesn't get as much review as we would
like, and areas that are primarily worked on by a single individual tend
to be worse in that regard. However, personally I don't feel that a
formal process is needed.
I'm very willing to have anyone point out concerns they may have with
any of my commits or even just request more detailed explanations. I
feel entitled to do the same for any commits that concern me.
I'm willing to review specific patches or revisions on request (subject
to time constraints) and I will continue to request comments or review
for specific changes I make that I feel merit the extra scrutiny.
Re: Jira workflow change ticket and code review process.
Posted by Robert Godfrey <ro...@gmail.com>.
> It sounds like consensus is building around commit-then-review with
> Jira, so let's go for that. I'll start a formal vote later today.
>
One thing I would like to see is some discussion about whether we will
all be following a common process or whether the components in each
language will be doing their own thing.
The process and meetings up until now have focused mainly on the Java
side of things; and we have been quite keen on enforcing things like
"no commit without an accompanying JIRA"...
I think if we want to have a formal commit-then-review process then
linking each commit to a JIRA will be necessary so that we can
document the review.
-- Rob
Re: Jira workflow change ticket and code review process.
Posted by Aidan Skinner <ai...@apache.org>.
On Mon, Aug 18, 2008 at 11:48 PM, Steve Huston <sh...@riverace.com> wrote:
>> From: Ted Ross [mailto:tross@redhat.com]
>> I think commit-then-review will be more efficient given the
>> capabilities of our tools.
>
> I agree with this as well, modulo two points. These may be covered
> already, but I haven't seen evidence of it...
>
> 1. If this process gets too far between commit and review, there's
> increased chance there will be changes add in after the commit that
> may confuse the reviewer.
Yeah, that's always a risk though. Not much we can really do about it
either way.
> 2. I haven't seen any sort of log or diary of changes. Something that
> would, for example, list the files changed and why. Is this normally
> put in the svn commit comments? Explanatory entries in such a place
> could help the reviewer get his/her bearings and help understand what
> the committer was trying to do. Or is this info expected to be in
> jira?
I would hope that this would be in the commit message.
> Also, is there someplace relatively easy to check to see if a
> particular commit broke something?
We don't have the Apache CruiseControl running on Qpid, although there
are CC conf's checked in. I keep meaning to engage with infra@ to get
us onto there, but haven't found the cycles yet.
It sounds like consensus is building around commit-then-review with
Jira, so let's go for that. I'll start a formal vote later today.
- 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: Jira workflow change ticket and code review process.
Posted by Steve Huston <sh...@riverace.com>.
> -----Original Message-----
> From: Ted Ross [mailto:tross@redhat.com]
>
> Robert Greig wrote:
> > OK, FWIW here's what I would do. No review board, just:
> >
> > 1) work on issue and commit changes
> > 2) mail or IM someone asking if he would mind reviewing it
> > 3) assign issue to person doing review and move on until issue is
> > either reopened by reviewer or resolved (or whatever state
> comes next)
> > 4) run jira report regularly of issues in "pending review"
> state with
> > no activity for 5 days. For those issues go back to step (2) with
> > alternative reviewer if necessary.
> >
> > I think it would be useful to get some of the other
> committers' views
> > on what would work for them.
> >
> > RG
> +1 to Robert's suggestion
>
> I think commit-then-review will be more efficient given the
> capabilities of our tools.
I agree with this as well, modulo two points. These may be covered
already, but I haven't seen evidence of it...
1. If this process gets too far between commit and review, there's
increased chance there will be changes add in after the commit that
may confuse the reviewer.
2. I haven't seen any sort of log or diary of changes. Something that
would, for example, list the files changed and why. Is this normally
put in the svn commit comments? Explanatory entries in such a place
could help the reviewer get his/her bearings and help understand what
the committer was trying to do. Or is this info expected to be in
jira?
Also, is there someplace relatively easy to check to see if a
particular commit broke something?
Thanks,
-Steve
Re: Jira workflow change ticket and code review process.
Posted by Ted Ross <tr...@redhat.com>.
Robert Greig wrote:
> OK, FWIW here's what I would do. No review board, just:
>
> 1) work on issue and commit changes
> 2) mail or IM someone asking if he would mind reviewing it
> 3) assign issue to person doing review and move on until issue is
> either reopened by reviewer or resolved (or whatever state comes next)
> 4) run jira report regularly of issues in "pending review" state with
> no activity for 5 days. For those issues go back to step (2) with
> alternative reviewer if necessary.
>
> I think it would be useful to get some of the other committers' views
> on what would work for them.
>
> RG
+1 to Robert's suggestion
I think commit-then-review will be more efficient given the capabilities
of our tools.
-Ted
Re: Jira workflow change ticket and code review process.
Posted by Robert Greig <ro...@gmail.com>.
2008/8/18 Aidan Skinner <ai...@apache.org>:
> That's why having a pool is better, if one person is busy with other
> stuff then others can pick up the load. If everybody is crunching like
> a maniac, it's probably a sign that careful review is even more
> necessary - I know the quality of my patches has an inverse
> relationship to the amount of Irn Bru consumed. ;)
I don't like the idea of a pool basically because of the lack of accountability.
> I don't think there's a significant difference in the toolset between
> commit-then-review and review-then-commit, and certainly not a
> compelling one that outweighs the other advantages of reviewing first.
OK, FWIW here's what I would do. No review board, just:
1) work on issue and commit changes
2) mail or IM someone asking if he would mind reviewing it
3) assign issue to person doing review and move on until issue is
either reopened by reviewer or resolved (or whatever state comes next)
4) run jira report regularly of issues in "pending review" state with
no activity for 5 days. For those issues go back to step (2) with
alternative reviewer if necessary.
I think it would be useful to get some of the other committers' views
on what would work for them.
RG
Re: Jira workflow change ticket and code review process.
Posted by Aidan Skinner <ai...@apache.org>.
On Mon, Aug 18, 2008 at 9:00 PM, Robert Greig <ro...@gmail.com> wrote:
> 2008/8/18 Aidan Skinner <ai...@apache.org>:
>
>> Not for post-commit reviews, at least no better than we currently
>> have. The good thing about pre-commit reviews in this instance is that
>> the onus is on the person assigned the bug to get it reviewed before
>> it goes in. With post-commit reviews there's a lot less technical
>> pressure to get it done, and it's harder to identify who's currently
>> responsible for reviewing it.
>
> I don't follow - surely we just assign the jira to the reviewer when
> the issue is in the "awaiting review" stage so you can immediately see
> who is responsible for reviewing it? Is that not the case irrespective
> of whether it is pre or post commit review?
Ah, that wasn't how I was envisaging this working, I've only ever seen
this implemented where you have a pool of reviewers who review
specific areas (I am, for instance, unlikely to review Ruby patches),
but the bug always remains assigned to the person doing the work.
Otherwise you end up bouncing the thing around a lot and it's not
always clear who's on the hook.
>> That requires a lot more intervention and may not scale as we build
>> diversity, review then commit places the responsibility for getting
>> the review done on the person doing the work and their peers, which
>> should be much more scalable.
>
> This is certainly true, the developer of the code will have an
> incentive to get the review done asap. Although the counter to that
> would be that someone who was otherwise snowed under may do a less
> thorough review than if he could do it at a time of his choosing. But
> whatever the process you can come up with scenarios like that.
That's why having a pool is better, if one person is busy with other
stuff then others can pick up the load. If everybody is crunching like
a maniac, it's probably a sign that careful review is even more
necessary - I know the quality of my patches has an inverse
relationship to the amount of Irn Bru consumed. ;)
>> In any case, it doesn't sound like you have fundamental objections to
>> review-then-commit, just practical concerns about the tooling?
>
> Yes that is correct. I'm just concerned that we will try to adopt a
> process that is impractical due in the main to limitations of our
> toolset and it will therefore break down.
That's why I suggested Review Board at the start, which is explicitly
designed to cope with this. Extra tooling does seem like a bit of a
burden though, I think we can probably survive with Jira+patch,
although it'll be a little irritating at times - there isn't really a
perfect solution available right now (git + review-board + a patch
queue bot that actually does the commits after building and running
the tests would be just about ideal I reckon, but would need a
significant chunk of infrastructure to support it).
I don't think there's a significant difference in the toolset between
commit-then-review and review-then-commit, and certainly not a
compelling one that outweighs the other advantages of reviewing first.
- 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: Jira workflow change ticket and code review process.
Posted by Robert Greig <ro...@gmail.com>.
2008/8/18 Aidan Skinner <ai...@apache.org>:
> Not for post-commit reviews, at least no better than we currently
> have. The good thing about pre-commit reviews in this instance is that
> the onus is on the person assigned the bug to get it reviewed before
> it goes in. With post-commit reviews there's a lot less technical
> pressure to get it done, and it's harder to identify who's currently
> responsible for reviewing it.
I don't follow - surely we just assign the jira to the reviewer when
the issue is in the "awaiting review" stage so you can immediately see
who is responsible for reviewing it? Is that not the case irrespective
of whether it is pre or post commit review?
>> The point is not that people follow different processes but that if
>> you built up a number of outstanding review items your boss can tell
>> you to do the reviews immediately.
>
> That requires a lot more intervention and may not scale as we build
> diversity, review then commit places the responsibility for getting
> the review done on the person doing the work and their peers, which
> should be much more scalable.
This is certainly true, the developer of the code will have an
incentive to get the review done asap. Although the counter to that
would be that someone who was otherwise snowed under may do a less
thorough review than if he could do it at a time of his choosing. But
whatever the process you can come up with scenarios like that.
> In any case, it doesn't sound like you have fundamental objections to
> review-then-commit, just practical concerns about the tooling?
Yes that is correct. I'm just concerned that we will try to adopt a
process that is impractical due in the main to limitations of our
toolset and it will therefore break down.
RG
Re: Jira workflow change ticket and code review process.
Posted by Aidan Skinner <ai...@apache.org>.
On Sat, Aug 16, 2008 at 10:58 PM, Robert Greig <ro...@gmail.com> wrote:
> at the best of times, and collective accountability generally results
> in no accountability. Once the process is integrated into the jira
> workflow (and this applies to either pre or post commit review), there
> will be a name in the frame so I think it is a different case.
Not for post-commit reviews, at least no better than we currently
have. The good thing about pre-commit reviews in this instance is that
the onus is on the person assigned the bug to get it reviewed before
it goes in. With post-commit reviews there's a lot less technical
pressure to get it done, and it's harder to identify who's currently
responsible for reviewing it.
>> All committers should follow the processes we agree here, regardless
>> of their employer. One Team, One Project, One Dream. ;)
>
> The point is not that people follow different processes but that if
> you built up a number of outstanding review items your boss can tell
> you to do the reviews immediately.
That requires a lot more intervention and may not scale as we build
diversity, review then commit places the responsibility for getting
the review done on the person doing the work and their peers, which
should be much more scalable.
> Yes, it will be interesting to see if svn ever manages to make the
> merge support good enough to make branch/merge a feasible strategy.
svk is pretty much there with this, at least from the 1/2 hour I gave
it last year.
In any case, it doesn't sound like you have fundamental objections to
review-then-commit, just practical concerns about the tooling?
- 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: Jira workflow change ticket and code review process.
Posted by Robert Greig <ro...@gmail.com>.
2008/8/16 Aidan Skinner <ai...@apache.org>:
> I suspect that we would keep up with it for a while, but once the
> initial surge of enthusiasm wears off it would build up and start to
> become problematic. I could be wrong, but that's been my experience in
> the past, both on this project and on others. It's basically what's
> happened with the weekly calls.
I think the weekly calls for reviewing stuff was a poor approach
doomed from the start - reviewing things on conference calls is awful
at the best of times, and collective accountability generally results
in no accountability. Once the process is integrated into the jira
workflow (and this applies to either pre or post commit review), there
will be a name in the frame so I think it is a different case.
>> of people getting paid to work on this project so those organisations
>> should be able to insist their staff follow the process.
>
> All committers should follow the processes we agree here, regardless
> of their employer. One Team, One Project, One Dream. ;)
The point is not that people follow different processes but that if
you built up a number of outstanding review items your boss can tell
you to do the reviews immediately.
> If svn gets better merging, that's great, and will make
> review-then-commit easier, since people can just take feature branches
> if they really, really need to get patches out of their tree and don't
> want to use git-svn. Hurrah!
Yes, it will be interesting to see if svn ever manages to make the
merge support good enough to make branch/merge a feasible strategy.
RG
Re: Jira workflow change ticket and code review process.
Posted by Aidan Skinner <ai...@apache.org>.
On Fri, Aug 15, 2008 at 9:21 PM, Robert Greig <ro...@gmail.com> wrote:
>> and I think review-then-commit
>> actually requires less discipline, and the majority of it is focussed
>> on the person with the biggest stake (the person writing the patch).
>
> The issue I have is that with the current toolset it is a pain and I
> know I would hate dealing with patch files on jiras. People will want
> the code committed so that things can progress (like dependent changes
> etc) and if there is any delay we may get into an annoying merge
> cycle.
Provided that reviews happen promptly, this shouldn't be a big
problem. Yes, there'll be a bit of a delay, but it shouldn't be huge
in the normal course of things. If people aren't rigirous in following
through, then there's immediate impetus for somebody to sort it out,
rather than just letting it rot. The pain caused by laziness is still
there, but it's up front and obvious and doesn't get a chance to build
up to the "oh good grief, we need to review a month of commits" level.
>> This should make it more likely that process is followed.
>
> I think we could determine how effective it is by monitoring the
> number of issues in the "review pending" state. We have a good number
I suspect that we would keep up with it for a while, but once the
initial surge of enthusiasm wears off it would build up and start to
become problematic. I could be wrong, but that's been my experience in
the past, both on this project and on others. It's basically what's
happened with the weekly calls.
> of people getting paid to work on this project so those organisations
> should be able to insist their staff follow the process.
All committers should follow the processes we agree here, regardless
of their employer. One Team, One Project, One Dream. ;)
>> svnmerge in 1.5 is better, but it's still nowhere up to the standards
>> of a proper DVCS (it still loses history, for one thing)
>
> My point was that at last it's making progress in this area. I haven't
> tested it out myself yet although I shall be doing so in the next
> couple of weeks.
If svn gets better merging, that's great, and will make
review-then-commit easier, since people can just take feature branches
if they really, really need to get patches out of their tree and don't
want to use git-svn. Hurrah!
- 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: Jira workflow change ticket and code review process.
Posted by Robert Greig <ro...@gmail.com>.
2008/8/15 Aidan Skinner <ai...@apache.org>:
> The review process is about discipline, but that's something that's
> traditionally somewhat lacking,
Without a workflow that supports it, it is definitely likely to slip.
> and I think review-then-commit
> actually requires less discipline, and the majority of it is focussed
> on the person with the biggest stake (the person writing the patch).
The issue I have is that with the current toolset it is a pain and I
know I would hate dealing with patch files on jiras. People will want
the code committed so that things can progress (like dependent changes
etc) and if there is any delay we may get into an annoying merge
cycle.
> This should make it more likely that process is followed.
I think we could determine how effective it is by monitoring the
number of issues in the "review pending" state. We have a good number
of people getting paid to work on this project so those organisations
should be able to insist their staff follow the process.
>> If we had a different SCM tool then maybe we would do it differently.
>> As another aside I see that svn 1.5.x does improve the merging
>> capabilities so maybe at last things will get better with that tool.
>
> svnmerge in 1.5 is better, but it's still nowhere up to the standards
> of a proper DVCS (it still loses history, for one thing)
My point was that at last it's making progress in this area. I haven't
tested it out myself yet although I shall be doing so in the next
couple of weeks.
RG
Re: Jira workflow change ticket and code review process.
Posted by Aidan Skinner <ai...@apache.org>.
On Thu, Aug 14, 2008 at 8:11 PM, Robert Greig <ro...@gmail.com> wrote:
> I agree with the above. I have always found patch files a total PITA,
> and in any case the review process is about discipline. We should
> allow people to commit and try to manage the reviews through reporting
The review process is about discipline, but that's something that's
traditionally somewhat lacking, and I think review-then-commit
actually requires less discipline, and the majority of it is focussed
on the person with the biggest stake (the person writing the patch).
This should make it more likely that process is followed.
> If we had a different SCM tool then maybe we would do it differently.
> As another aside I see that svn 1.5.x does improve the merging
> capabilities so maybe at last things will get better with that tool.
svnmerge in 1.5 is better, but it's still nowhere up to the standards
of a proper DVCS (it still loses history, for one thing)
- 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: Jira workflow change ticket and code review process.
Posted by Robert Greig <ro...@gmail.com>.
2008/8/14 Robert Godfrey <ro...@gmail.com>:
>>
>> Potential patch conflicts are less of a problem from my PoV than
>> having a ton of Jiras and commits that haven't gotten past the "ready
>> for review" stage because people are inherently lazy and it's easier
>> to not do it.
>
> But the likelihood is that instead you'd get a load of JIRAs with
> patches on that no-one reviews because they are lazy :-)
>
> I think that if we as a group are good at enforcing the review phase
> then we should be able to make this process workable. Producing
> automated reports for number of unreviewed commits - plus a summary of
> how many reviews each developer has done in the past month should I
> think be sufficient to keep the number of unreviewed changes down to a
> minimum.
I agree with the above. I have always found patch files a total PITA,
and in any case the review process is about discipline. We should
allow people to commit and try to manage the reviews through reporting
and nudging. Reviewing commits will also reduce the time for the
reviewer I think, which streamlines the process.
If we had a different SCM tool then maybe we would do it differently.
As another aside I see that svn 1.5.x does improve the merging
capabilities so maybe at last things will get better with that tool.
RG
Re: Jira workflow change ticket and code review process.
Posted by Aidan Skinner <ai...@apache.org>.
On Thu, Aug 14, 2008 at 6:46 PM, Robert Godfrey <ro...@gmail.com> wrote:
> But the likelihood is that instead you'd get a load of JIRAs with
> patches on that no-one reviews because they are lazy :-)
But then there's the impetus for the owner to nudge a reviewer, and
there's also social pressure to not hold up the other person by
reviewing promptly and properly.
I've seen review-then-commit work well as normal process in other
projects (eg. GDB) where you have a largeish number of developers, and
as a 'coming up for release' thing in projects where you have a
smaller number of developers (eg. GNOME, Red Carpet).
I've never seen commit-then-review work for any length of time,
there's an initial flurry and then it dies down as people go on
holiday, get interrupted or just get busy. We were doing
commit-then-review before, and AFAICT, the review part just didn't
happen all that much.
> I think that if we as a group are good at enforcing the review phase
> then we should be able to make this process workable. Producing
> automated reports for number of unreviewed commits - plus a summary of
> how many reviews each developer has done in the past month should I
> think be sufficient to keep the number of unreviewed changes down to a
> minimum.
Reports work, but as we've seen with the existing process, actions are
frequently left to languish on the "todo list". Even taking into
account the synchronous nature of the review process we had, I still
don't believe that people will do it.
Automated reports are likely to be met with a "oh, yeah, I should do
that, sorry...", but since we're responsible for reviewing things as a
project it's unlikely that anybody will actually take personal
ownership of that.
- 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: Jira workflow change ticket and code review process.
Posted by Robert Godfrey <ro...@gmail.com>.
>
> Potential patch conflicts are less of a problem from my PoV than
> having a ton of Jiras and commits that haven't gotten past the "ready
> for review" stage because people are inherently lazy and it's easier
> to not do it.
But the likelihood is that instead you'd get a load of JIRAs with
patches on that no-one reviews because they are lazy :-)
I think that if we as a group are good at enforcing the review phase
then we should be able to make this process workable. Producing
automated reports for number of unreviewed commits - plus a summary of
how many reviews each developer has done in the past month should I
think be sufficient to keep the number of unreviewed changes down to a
minimum.
-- Rob
Re: Jira workflow change ticket and code review process.
Posted by Aidan Skinner <ai...@apache.org>.
On Thu, Aug 14, 2008 at 4:25 PM, Robert Godfrey <ro...@gmail.com> wrote:
> I don't think review then commit is going to work personally. For a
> start you now have a three way exchange before a commit can be made
> (put on JIRA... get reveiwed... wait for OK... commit)... and the
> latency introduced will generate a much greater chance of patches
> failing due to conflicts... While I agree a git based source control
I would hope that this would be relatively rare, and while it does
introduce some latency it does also ensure that the patch has been
reviewed before it goes in.
Potential patch conflicts are less of a problem from my PoV than
having a ton of Jiras and commits that haven't gotten past the "ready
for review" stage because people are inherently lazy and it's easier
to not do it.
> system would make this feasible... we are not in such a world. The
> majority continue to use subversion and that is the system Apache is
> providing us.
I'm not suggesting that we mandate it, but we could all switch to
using git over svn. It was mostly just an idle comment ;)
- 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: Jira workflow change ticket and code review process.
Posted by Robert Godfrey <ro...@gmail.com>.
2008/8/14 Aidan Skinner <ai...@apache.org>:
> On Thu, Aug 14, 2008 at 3:21 PM, Andrew Stitcher <as...@redhat.com> wrote:
>
>> On Thu, 2008-08-14 at 15:16 +0100, Aidan Skinner wrote:
>>> I've raised an INFRA ticket to get a 'Ready for review' status in
>>> between 'In Progress' and 'Resolved'.
>>>
>>> I'd quite like to move the Java client and broker to a
>>> review-then-commit model, with any one committer other than the author
>>> being sufficent.
>>
>> 1. I assume this is intended for changes to released code.
>
> I was kinda thinking for all the Java stack actually. We don't do much
> that is entirely new development.
>
Personally I think in general it is good to have some form of
acknowledged review of all code that gets committed. Since we do not
operate a policy of benevolent dictators who review all before
committing I support the step of having a documented review step in
the JIRA process.
>> 2. Without actually commiting the change the reviewer doesn't easily get
>> to see the change, and no I don't consider attached patches to be
>> adequate with svn.
>
> Why not? Is it just the rebasing problem?
>
> Personally I'd prefer small branches that can be reviewed then merged,
> but I don't think that's realistic until and unless we move to
> $DVCS[1].
>
I don't think review then commit is going to work personally. For a
start you now have a three way exchange before a commit can be made
(put on JIRA... get reveiwed... wait for OK... commit)... and the
latency introduced will generate a much greater chance of patches
failing due to conflicts... While I agree a git based source control
system would make this feasible... we are not in such a world. The
majority continue to use subversion and that is the system Apache is
providing us.
-- Rob
> - Aidan
>
> [1] For $DVCS == git, as it is clearly superior to bzr and hg in crucial ways
> --
> 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: Jira workflow change ticket and code review process.
Posted by Aidan Skinner <ai...@apache.org>.
On Thu, Aug 14, 2008 at 3:21 PM, Andrew Stitcher <as...@redhat.com> wrote:
> On Thu, 2008-08-14 at 15:16 +0100, Aidan Skinner wrote:
>> I've raised an INFRA ticket to get a 'Ready for review' status in
>> between 'In Progress' and 'Resolved'.
>>
>> I'd quite like to move the Java client and broker to a
>> review-then-commit model, with any one committer other than the author
>> being sufficent.
>
> 1. I assume this is intended for changes to released code.
I was kinda thinking for all the Java stack actually. We don't do much
that is entirely new development.
> 2. Without actually commiting the change the reviewer doesn't easily get
> to see the change, and no I don't consider attached patches to be
> adequate with svn.
Why not? Is it just the rebasing problem?
Personally I'd prefer small branches that can be reviewed then merged,
but I don't think that's realistic until and unless we move to
$DVCS[1].
- Aidan
[1] For $DVCS == git, as it is clearly superior to bzr and hg in crucial ways
--
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: Jira workflow change ticket and code review process.
Posted by Andrew Stitcher <as...@redhat.com>.
On Thu, 2008-08-14 at 15:16 +0100, Aidan Skinner wrote:
> I've raised an INFRA ticket to get a 'Ready for review' status in
> between 'In Progress' and 'Resolved'.
>
> I'd quite like to move the Java client and broker to a
> review-then-commit model, with any one committer other than the author
> being sufficent.
1. I assume this is intended for changes to released code.
2. Without actually commiting the change the reviewer doesn't easily get
to see the change, and no I don't consider attached patches to be
adequate with svn.
Andrew (turning up to gas mark 1)
>
> - Aidan (who is donning his asbestos suit)