Clarifications on Code review efficiency

[Ben Goodger]

Team,

If you don't review code in Chromium you can skip reading now.

Code review is an important part of our development process. I'd like to ask everyone keep the following things in mind:

• We should strive to respond promptly to activity on code reviews. We should aim to provide some kind of actionable response within 24 hours of receipt.
  
  This doesn't mean you have to have done a complete review as in some cases that's not possible, it might just be a request for some more time or to suggest another reviewer.
  
  I personally get a huge amount of email and CRs often slip through the cracks, so I tell people they shouldn't feel bad about pinging me via IM after 24hrs. I think this is an acceptable practice.
  
  Of course please be cognizant of timezone differences, vacations, national holidays, travel schedules etc. As always, use your best judgement and common sense rules prevail.
  
  
• OWNERS files should contain only people actively reviewing code in that directory. I've seen a a few instances of "emeritus" owners which I don't think is helpful & so we should end this practice.
  
  
• While it's a great idea to indicate your availability status in your Rietveld name (e.g. OOO, traveling etc, etc.), just placing a blanket "slow" because you feel overwhelmed by the number of reviews you're receiving isn't a good solution.
  
  Please work with your manager and tech lead(s) to help expand OWNERS in your area over time.

Thanks and let's keep the code flowing!

-Ben

[Peter Kasting]

On Thu, Dec 1, 2016 at 12:07 PM, Ben Goodger <b...@chromium.org> wrote:

• While it's a great idea to indicate your availability status in your Rietveld name (e.g. OOO, traveling etc, etc.), just placing a blanket "slow" because you feel overwhelmed by the number of reviews you're receiving isn't a good solution.

You mean, as a long-term rate-limit on reviews, as opposed to a short-term thing (e.g. after my last OOO period, I marked myself as "slow" for 2 days while I dug out of the hole)?

PK 

[Charles Harrison]

Is there an automated process that could detect inactive owners and file bugs to remove them? I find I often waste considerable time double checking reviewers history to ensure they are active.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Ben Goodger]

I think the thing is it's not always clear that it's temporary. As always, over communication is helpful.

-Ben

[Nico Weber]

On Thu, Dec 1, 2016 at 3:07 PM, Ben Goodger <b...@chromium.org> wrote:

Team,

If you don't review code in Chromium you can skip reading now.

Code review is an important part of our development process. I'd like to ask everyone keep the following things in mind:

• We should strive to respond promptly to activity on code reviews. We should aim to provide some kind of actionable response within 24 hours of receipt.
  
  This doesn't mean you have to have done a complete review as in some cases that's not possible, it might just be a request for some more time or to suggest another reviewer.
  
  I personally get a huge amount of email and CRs often slip through the cracks, so I tell people they shouldn't feel bad about pinging me via IM after 24hrs. I think this is an acceptable practice.
  
  Of course please be cognizant of timezone differences, vacations, national holidays, travel schedules etc. As always, use your best judgement and common sense rules prevail.
  
  
• OWNERS files should contain only people actively reviewing code in that directory. I've seen a a few instances of "emeritus" owners which I don't think is helpful & so we should end this practice.
  

When I proposed this a while ago there was quite a bit of pushback: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/zS9ZBdFiYJM/DO5OahxblOcJ I assume jam/darin are now on board with this? (If so, hooray!)

• While it's a great idea to indicate your availability status in your Rietveld name (e.g. OOO, traveling etc, etc.), just placing a blanket "slow" because you feel overwhelmed by the number of reviews you're receiving isn't a good solution.
  
  Please work with your manager and tech lead(s) to help expand OWNERS in your area over time.

Thanks and let's keep the code flowing!

-Ben

[Peter Kasting]

On Thu, Dec 1, 2016 at 12:15 PM, Ben Goodger <b...@chromium.org> wrote:

I think the thing is it's not always clear that it's temporary. As always, over communication is helpful.

Rietveld names are severely length-limited.  If poly-gerrit increases those limits, or adds an explicit status field, that would help provide more space to clarify (assuming such clarification is desirable).

PK

[Marijn Kruisselbrink]

Would it make sense to somehow link this to the (recently introduced) feature in monorail to add status fields to users and have some indicator of people being somewhat active?

PK

[Peter Kasting]

On Thu, Dec 1, 2016 at 12:30 PM, Marijn Kruisselbrink <m...@chromium.org> wrote:

On Thu, Dec 1, 2016 at 12:26 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Thu, Dec 1, 2016 at 12:15 PM, Ben Goodger <b...@chromium.org> wrote:

I think the thing is it's not always clear that it's temporary. As always, over communication is helpful.

Rietveld names are severely length-limited.  If poly-gerrit increases those limits, or adds an explicit status field, that would help provide more space to clarify (assuming such clarification is desirable).

Would it make sense to somehow link this to the (recently introduced) feature in monorail to add status fields to users and have some indicator of people being somewhat active?

I think it would make a great deal of sense :).

PK 

[Ben Goodger]

Please talk to our friends in polygerrit land about improving availability info. There are so many interesting ways the tool could help here.

-Ben

[Dirk Pranke]

On Thu, Dec 1, 2016 at 12:19 PM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Dec 1, 2016 at 3:07 PM, Ben Goodger <b...@chromium.org> wrote:

• OWNERS files should contain only people actively reviewing code in that directory. I've seen a a few instances of "emeritus" owners which I don't think is helpful & so we should end this practice.
  

When I proposed this a while ago there was quite a bit of pushback: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/zS9ZBdFiYJM/DO5OahxblOcJ I assume jam/darin are now on board with this? (If so, hooray!)

I think I posted the last message in that thread, and while it probably could've been construed as pushback, I *think* I just meant to clarify the intent of the actual implementation and policy, rather than what I thought the policy *should* be.

But, if you were to ask me what the policy should be, these days I'd strongly agree with what Ben wrote.

Some people have expressed the idea that it's a sign of respect to leave contributors in. I could agree with that, but I think the value of that decays over time. If someone left the project months or years ago, or the code has evolved significantly since the last time said person worked on it, I think the tradeoff shifts in favor of removing them. 

On Thu, Dec 1, 2016 at 12:15 PM, Charles Harrison <cshar...@chromium.org> wrote:

Is there an automated process that could detect inactive owners and file bugs to remove them? I find I often waste considerable time double checking reviewers history to ensure they are active.

We do not have an automated process. I believe various people have been looking into the distribution of reviews and reviewers lately, and I suspect we will end up doing *something* fairly soon to clean up the entries. It's certainly not hard to do that. Whether we need to do something manually once in a while or more regularly is surely something we could figure out down the road.

-- Dirk