Pull requests : speed up the reviews

Pull requests : speed up the reviews

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

Hi Ceph,

In the past few weeks the number of pending pull requests grew from around 20 to over 80. The good thing is that there are more contributions, the problem is that it requires more reviewers. Ceph is not the only project suffering from this kind of problem and attending the OpenStack summit last week reminded me that the sooner it is addressed the better.

After a few IRC discussions some ideas came up and my favorite is that every developer paid full time to work on Ceph dedicates a daily 15 minutes time slot, time boxed, to review pull requests. Timeboxing is kind of frustrating because some reviews require more. It basically means one has to focus on the pull request for ten minutes at most and take five minutes to write a useful comment that helps the author moving forward. But it also is the only way to make room for a daily activity with no risk of postponing it because something more urgent came up.

What do you think ?

Cheers

-- 
Loïc Dachary, Artisan Logiciel Libre


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: Pull requests : speed up the reviews

[Joao Eduardo Luis]

On 11/08/2014 05:32 PM, Loic Dachary wrote:
> Hi Ceph,
>
> In the past few weeks the number of pending pull requests grew from around 20 to over 80. The good thing is that there are more contributions, the problem is that it requires more reviewers. Ceph is not the only project suffering from this kind of problem and attending the OpenStack summit last week reminded me that the sooner it is addressed the better.
>
> After a few IRC discussions some ideas came up and my favorite is that every developer paid full time to work on Ceph dedicates a daily 15 minutes time slot, time boxed, to review pull requests. Timeboxing is kind of frustrating because some reviews require more. It basically means one has to focus on the pull request for ten minutes at most and take five minutes to write a useful comment that helps the author moving forward. But it also is the only way to make room for a daily activity with no risk of postponing it because something more urgent came up.
>
> What do you think ?

On my calendar, I do have a time slot of one hour each morning to review 
pull requests and mailing lists but I seldom honor it, especially when 
I'm caught up in other stuff.

I'll move it over to lunch so that it has no chance in interfering with 
other tasks and try to make a habit of it.

It would also be interesting to see more community involvement.  I 
believe it would be healthy for the project if we could have (at least) 
a portion of reviews being performed by other people besides solely the 
paid developers/maintainers.

   -Joao

-- 
Joao Eduardo Luis
Software Engineer | http://ceph.com

Re: Pull requests : speed up the reviews

[David Zafman]

I know I had a couple of pull requests that we weren’t going to merge until after the giant release.  This may have applied some of the other ones too.  In addition, It isn’t surprising that with a new release some non-release code reviews would be neglected.

That being said, this is a good time to remind people to dedicate time to code reviews.

David Zafman
Senior Developer
http://www.inktank.com




> On Nov 9, 2014, at 4:08 AM, Joao Eduardo Luis <joao@redhat.com> wrote:
> 
> On 11/08/2014 05:32 PM, Loic Dachary wrote:
>> Hi Ceph,
>> 
>> In the past few weeks the number of pending pull requests grew from around 20 to over 80. The good thing is that there are more contributions, the problem is that it requires more reviewers. Ceph is not the only project suffering from this kind of problem and attending the OpenStack summit last week reminded me that the sooner it is addressed the better.
>> 
>> After a few IRC discussions some ideas came up and my favorite is that every developer paid full time to work on Ceph dedicates a daily 15 minutes time slot, time boxed, to review pull requests. Timeboxing is kind of frustrating because some reviews require more. It basically means one has to focus on the pull request for ten minutes at most and take five minutes to write a useful comment that helps the author moving forward. But it also is the only way to make room for a daily activity with no risk of postponing it because something more urgent came up.
>> 
>> What do you think ?
> 
> On my calendar, I do have a time slot of one hour each morning to review pull requests and mailing lists but I seldom honor it, especially when I'm caught up in other stuff.
> 
> I'll move it over to lunch so that it has no chance in interfering with other tasks and try to make a habit of it.
> 
> It would also be interesting to see more community involvement.  I believe it would be healthy for the project if we could have (at least) a portion of reviews being performed by other people besides solely the paid developers/maintainers.
> 
>  -Joao
> 
> -- 
> Joao Eduardo Luis
> Software Engineer | http://ceph.com
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Pull requests : speed up the reviews

[Gregory Farnum]

On Sat, Nov 8, 2014 at 9:32 AM, Loic Dachary <loic@dachary.org> wrote:
> Hi Ceph,
>
> In the past few weeks the number of pending pull requests grew from around 20 to over 80. The good thing is that there are more contributions, the problem is that it requires more reviewers. Ceph is not the only project suffering from this kind of problem and attending the OpenStack summit last week reminded me that the sooner it is addressed the better.
>
> After a few IRC discussions some ideas came up and my favorite is that every developer paid full time to work on Ceph dedicates a daily 15 minutes time slot, time boxed, to review pull requests. Timeboxing is kind of frustrating because some reviews require more. It basically means one has to focus on the pull request for ten minutes at most and take five minutes to write a useful comment that helps the author moving forward. But it also is the only way to make room for a daily activity with no risk of postponing it because something more urgent came up.
>
> What do you think ?

This is something I've already been thinking about a bit for different
reasons. Historically we've had a very informal and organic approach
to review management.
When reviews first started, each developer went and sought out
somebody they felt was appropriate to review the code change. As the
team grew and we started to get more external contributions, this
basically meant that every review funneled through the tech leads:
leads selected reviewers for self-written code and then for every
other PR either reviewed it or explicitly passed it off to a competent
reviewer. Code which didn't fit into a clear lead category were
handled by Sage or occasionally somebody else (*self-aggrandizing
cough*). This used to work, but we're seeing its limitations as we
grow.
At the moment it's a particular concern because Sage and Sam have been
focused on the Giant release, I've been slowly but ruthlessly pruning
off activities like community review in the service of cephfs fsck
(and John and Zheng's productivity!), and we're getting a *lot* more
PRs of substance than we used to.

So we need a new system (or to be more proactive about our existing
informal system) for managing reviews. I think there are a few things
that a review process needs to address:
1) New PRs need to get an appropriate amount of commentary quickly.
2) PRs need to be reviewed by a competent developer prior to merge.
3) Reviewing needs to scale.

An "appropriate amount of commentary" obviously depends on the nature
of the review: we want to be more proactive with new contributors;
small but important patches should be handled more quickly than
long-term major features; a patch which isn't reviewable for style
reasons doesn't need the line-by-line attention of something that we
would like to merge ASAP.
The identity of a "competent developer" varies a lot across different
parts of the project, but I think this might be the key issue: there
are a lot of things where the pool of already-qualified people is only
one or two deep.

I suppose one way of handling this might be to ask everybody to
dedicate a small amount of time to reviews (as you suggest), but to
emphasize PR management as much as doing the actual review.
1) If you don't have assigned PRs, look at the unassigned PRs from
oldest to newest, and based on the target area assign it to the person
you think most appropriate.after doing basic due diligence for issues
that you can handle (ie, the feature is something you don't know is
inappropriate; skimming the code doesn't make you cry; etc)
2) Manage any reviews assigned to you:
  2a) if you're the wrong target, assign it to somebody more appropriate
  2b) If you're the right target but are trying to get somebody else
acquainted with the code base, maybe assign it to them
  2c) assign priorities based on importance and age
3) review PRs in appropriate order at the level they deserve, and then
either merge or assign it back to the author

In particular, this clearly assigns responsibility for a PR to an
individual. It lets people offload as necessary, and gives feedback to
contributors quickly even if that feedback is just "we appreciate your
submission and it is in somebody's queue". It lets people contribute
to the PR process even if there isn't something they feel qualified to
review directly. And it doesn't emphasize the time spent reviewing so
much as the act of moving a PR through the stages of merging. (A
15-minute timebox might sound great, but trust me we have lots of PRs
that would never make it into the code base if they were done in
15-minute chunks.)

Obviously this is also not a very formal process, but I think maybe if
we start approaching things this way it will help move stuff through
the pipeline faster. Based on Sage's sudden enthusiasm for assigning
PR owners I think he might agree...?
-Greg

Re: Pull requests : speed up the reviews

[Sage Weil]

On Mon, 10 Nov 2014, Gregory Farnum wrote:
> I suppose one way of handling this might be to ask everybody to
> dedicate a small amount of time to reviews (as you suggest), but to
> emphasize PR management as much as doing the actual review.
>
> 1) If you don't have assigned PRs, look at the unassigned PRs from
> oldest to newest, and based on the target area assign it to the person
> you think most appropriate.after doing basic due diligence for issues
> that you can handle (ie, the feature is something you don't know is
> inappropriate; skimming the code doesn't make you cry; etc)
>
> 2) Manage any reviews assigned to you:
>   2a) if you're the wrong target, assign it to somebody more appropriate
>   2b) If you're the right target but are trying to get somebody else
> acquainted with the code base, maybe assign it to them
>   2c) assign priorities based on importance and age
>
> 3) review PRs in appropriate order at the level they deserve, and then
> either merge or assign it back to the author
> 
> In particular, this clearly assigns responsibility for a PR to an
> individual. It lets people offload as necessary, and gives feedback to
> contributors quickly even if that feedback is just "we appreciate your
> submission and it is in somebody's queue". It lets people contribute
> to the PR process even if there isn't something they feel qualified to
> review directly. And it doesn't emphasize the time spent reviewing so
> much as the act of moving a PR through the stages of merging. (A
> 15-minute timebox might sound great, but trust me we have lots of PRs
> that would never make it into the code base if they were done in
> 15-minute chunks.)
> 
> Obviously this is also not a very formal process, but I think maybe if
> we start approaching things this way it will help move stuff through
> the pipeline faster. Based on Sage's sudden enthusiasm for assigning
> PR owners I think he might agree...?

+1

:) sage

Re: Pull requests : speed up the reviews

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 2210 bytes --]

Hi Greg,

> On Mon, 10 Nov 2014, Gregory Farnum wrote:
>> I suppose one way of handling this might be to ask everybody to
>> dedicate a small amount of time to reviews (as you suggest), but to
>> emphasize PR management as much as doing the actual review.
>>
>> 1) If you don't have assigned PRs, look at the unassigned PRs from
>> oldest to newest, and based on the target area assign it to the person
>> you think most appropriate.after doing basic due diligence for issues
>> that you can handle (ie, the feature is something you don't know is
>> inappropriate; skimming the code doesn't make you cry; etc)
>>
>> 2) Manage any reviews assigned to you:
>>   2a) if you're the wrong target, assign it to somebody more appropriate
>>   2b) If you're the right target but are trying to get somebody else
>> acquainted with the code base, maybe assign it to them
>>   2c) assign priorities based on importance and age
>>
>> 3) review PRs in appropriate order at the level they deserve, and then
>> either merge or assign it back to the author

I'll do as you suggest.

>> In particular, this clearly assigns responsibility for a PR to an
>> individual. It lets people offload as necessary, and gives feedback to
>> contributors quickly even if that feedback is just "we appreciate your
>> submission and it is in somebody's queue". It lets people contribute
>> to the PR process even if there isn't something they feel qualified to
>> review directly. And it doesn't emphasize the time spent reviewing so
>> much as the act of moving a PR through the stages of merging. (A
>> 15-minute timebox might sound great, but trust me we have lots of PRs
>> that would never make it into the code base if they were done in
>> 15-minute chunks.)

I see what you mean. However there are few pull requests of that kind in the queue, or am I understimating most of them ?

>> Obviously this is also not a very formal process, but I think maybe if
>> we start approaching things this way it will help move stuff through
>> the pipeline faster. Based on Sage's sudden enthusiasm for assigning
>> PR owners I think he might agree...?

+1

-- 
Loïc Dachary, Artisan Logiciel Libre


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: Pull requests : speed up the reviews

[Gregory Farnum]

On Mon, Nov 10, 2014 at 11:50 AM, Loic Dachary <loic@dachary.org> wrote:
> Hi Greg,
>
>> On Mon, 10 Nov 2014, Gregory Farnum wrote:
>>> I suppose one way of handling this might be to ask everybody to
>>> dedicate a small amount of time to reviews (as you suggest), but to
>>> emphasize PR management as much as doing the actual review.
>>>
>>> 1) If you don't have assigned PRs, look at the unassigned PRs from
>>> oldest to newest, and based on the target area assign it to the person
>>> you think most appropriate.after doing basic due diligence for issues
>>> that you can handle (ie, the feature is something you don't know is
>>> inappropriate; skimming the code doesn't make you cry; etc)
>>>
>>> 2) Manage any reviews assigned to you:
>>>   2a) if you're the wrong target, assign it to somebody more appropriate
>>>   2b) If you're the right target but are trying to get somebody else
>>> acquainted with the code base, maybe assign it to them
>>>   2c) assign priorities based on importance and age
>>>
>>> 3) review PRs in appropriate order at the level they deserve, and then
>>> either merge or assign it back to the author
>
> I'll do as you suggest.
>
>>> In particular, this clearly assigns responsibility for a PR to an
>>> individual. It lets people offload as necessary, and gives feedback to
>>> contributors quickly even if that feedback is just "we appreciate your
>>> submission and it is in somebody's queue". It lets people contribute
>>> to the PR process even if there isn't something they feel qualified to
>>> review directly. And it doesn't emphasize the time spent reviewing so
>>> much as the act of moving a PR through the stages of merging. (A
>>> 15-minute timebox might sound great, but trust me we have lots of PRs
>>> that would never make it into the code base if they were done in
>>> 15-minute chunks.)
>
> I see what you mean. However there are few pull requests of that kind in the queue, or am I understimating most of them ?

I don't know; see my comment about ruthlessly pruning things above. ;)
But from my recent queue/things I noticed/things I'm seeing in the
queue now:
1) Lots of MDS changes of substance — osd epoch barrier handling, the
inode scrub stuff, locking format changes, quotas
2) The AsyncMessenger, which has merged as an experimental feature but
still not seen formal review
3) copy-on-read rbd changes, rbd bitmaps

(VERY IMPORTANT) Keep in mind that part of the job of reviewing
includes making sure that any suggested changes have been through
adequate testing. For stuff from full-time contributors you work with
daily, that might just be poking them to document it or remembering
that they discussed it in a standup. But with external users it often
means either persuading them to set up teuthology, or developing a
testing script for them — or if you're lucky, simply pulling their
branch into the upstream repo and scheduling a suite run. :( We get a
lot of stuff that is trickier than it looks and either hasn't been
tested or has only been tested under working conditions, not failure
conditions; that needs to be covered before merging.
-Greg
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Pull requests : speed up the reviews

[Sage Weil]

We spent some time going through the backlog yesterday and made quite a 
bit of progress (down to 57 open).  A few changes:

- got rid of the firefly, giant, dumpling, etc. tags
- added firefly, giant, dumpling, hammer, next 'milestones'
- rgw and rbd projects have all items assigned
- only 14 unassigned

I also created a wip-sam-testing label to make it a bit easier to track 
what Sam is currently testing (tho nothing is tagged that way yet).

sage