On Mon, Nov 10, 2014 at 11:50 AM, Loic Dachary <loic@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
