PSA: TBR is for fixing build breaks + reverts, not normal development. Also, passing try jobs or CQ must be used even for simple-looking changes.

[John Abd-El-Malek]

(I've seen this a few times recently)

Please never land a change that hasn't been reviewed. This is strongly against the development practice of Chromium.

The only acceptable uses of TBR are:

-fixing a tree breakage (i.e. because a configuration is broken which isn't run on the try server)

-reverts/merges

-TBRing owners of consumers of API changes which has been reviewed by owners of that API

TBR should never be used for

-really simple changes that you think don't need to be reviewed

-making a change that someone said should be done. they need to review it.

-a reviewer/owner is away

Likewise, other than fixing a tree breakage or a revert, a change should never land without green try runs or through the CQ. Obviously there's a judgement call when the try runs are failing because of a flaky test, but we trust devs to make that call. However committing a change manually without try runs, or before the try runs complete, or through NOTRY=true, means that a judgement call wasn't even made. Even simple changes can break the tree in ways that we can't imagine, so never assume that if a change is really small it doesn't need to be tried. The waterfall is not a try server!

[James Hawkins]

Thank you for sending this out, John.  Respecting the owners system is critical to maintaining the health of the code base.

The most common TBR I receive as an owner is for a CL considered by the creator and non-owner reviewers as 'trivial'.  Trivial is subjective, and trivial changes can create catastrophic bugs as easily as a longer, more complex CL.  Please respect the owner system and wait for owner approval.

Thanks,

James

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
 
 
 

[Lei Zhang]

Also, please include the people in the TBR= line on the reviewer list
for your CLs. I often see CLs TBR'd to me where I never got an email.

On Tue, Mar 26, 2013 at 1:45 PM, John Abd-El-Malek <j...@chromium.org> wrote:

[Dominic Mazzoni]

The flip side to this is that if you are an OWNER, please respond to code reviews promptly. If it really is trivial, you should be able to approve it within 1 day. If it's not, please just respond and let the author know when you'll have a chance to take a look. It's extremely frustrating to wait 4 - 5 days for an OWNER to review <10 lines of code.

- Dominic

[Isaac Levy]

John mentioned an idea to mitigate the "my CL doesn't need trybots" mentality:

If a developer commits a change without green tryjobs and subsequently breaks the tree (in a way that would have been prevented by tryjobs), they get a point.

At the end of the month, we tabulate the points and the worst offenders get extra sheriff days.

Thus those that break the tree in a preventable manner are tasked with cleaning up the tree.

WDYT?

-Isaac

[Rachel Blum]

Not sure if I want to have the people who break the tree in the first place in charge of keeping it sane.

But maybe we can convince maruel to add another version of http://chromium-status.appspot.com/cq/top :)

 - rachel

[Thiago Farina]

On Tue, Mar 26, 2013 at 6:28 PM, Rachel Blum <gr...@chromium.org> wrote:
> Not sure if I want to have the people who break the tree in the first place
> in charge of keeping it sane.
>

It would force them to see the other side of the coin :)

--
Thiago

[Mark Larson]

On Tue, Mar 26, 2013 at 2:17 PM, Isaac Levy <il...@chromium.org> wrote:

John mentioned an idea to mitigate the "my CL doesn't need trybots" mentality:

If a developer commits a change without green tryjobs and subsequently breaks the tree (in a way that would have been prevented by tryjobs), they get a point.

At the end of the month, we tabulate the points and the worst offenders get extra sheriff days.

Thus those that break the tree in a preventable manner are tasked with cleaning up the tree.

WDYT?

I think this thread touches on a couple of issues that matter to me.

1. Get code review for every change. As John points out, this is about following Chromium practices. Code review and the watchlists that get triggered are an important practice in keeping a project as large and complex as ours healthy. It's not just about not breaking the tree or skipping try bots, though those are also things we need to keep the project healthy. 

2. Sheriffing is not punishment. It's a responsibility, almost a privilege, that allows people who have a stake in the success of the project to participate in keeping it healthy. The waterfall is like the project's immune system, detecting and rejecting pathogens that creep in. The sheriff is the physician on call, there to respond when the immune system flares up with red fever, nausea, or vomiting and take action to get the system healthy again. There are benefits to working on a large open source project, and those come with some responsibility to help keep the project healthy. 

I'd like to keep discussions focused on making it as easy as possible for people to do the right thing rather than punishing people for doing the wrong thing. Chromium has been very successful thanks to all the committers who care about doing the right thing. 

Anyway, I get concerned whenever the "sheriffing is punishment" meme crops up. 

--Mark

[Alexander Potapenko]

I've had a CL recently that had been reviewed, committed, but then
reverted because an extra file was occasionally included into that CL.
When the time came to re-land that CL again, it didn't change much - I
just removed that file from the patch.
Was it fine to TBR the people that had reviewed the CL already (I
actually did so)?

> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev
>
>
>



--

Alexander Potapenko
Software Engineer
Google Moscow

[Mattias Nissler]

I think it's good that you re-state the rule here: From seeing TBRs in this category fly by many people probably extrapolated (and maybe unconsciously so) that trivial changes are OK to TBR in general, which they are not.

>
> TBR should never be used for
> -really simple changes that you think don't need to be reviewed
> -making a change that someone said should be done. they need to review it.

I've seen this a couple times. And some of these attempts broke the tree :)

Also, I've seen reviews where the reviewer was obviously being too permissive because of a "that-author-is-a-big-shot-what-he-does-must-be-right" attitude. Please don't do that either. I think we should err on the side of questioning code when in doubt (and junior reviewers might as well learn something in the process). That's definitely favorable over fixing bugs later in the process that the reviewer could have caught.

[John Abd-El-Malek]

On Wed, Mar 27, 2013 at 1:09 AM, Alexander Potapenko <gli...@chromium.org> wrote:

I've had a CL recently that had been reviewed, committed, but then
reverted because an extra file was occasionally included into that CL.

I'm not sure what occassionally included means?

 

When the time came to re-land that CL again, it didn't change much - I
just removed that file from the patch.
Was it fine to TBR the people that had reviewed the CL already (I
actually did so)?

I think this is another case of judgement, and I agree that if files by owners didn't change the second time around, no need to ask them to look at it again. Of course, someone needs to review the changes from the first commit to the second commit. 

[Alexander Potapenko]

>
> I'm not sure what occassionally included means?
>

There was a hunk touching some file unrelated to the change, and I've
occasionally included that file into the list of files in the CL when
doing gcl change.

>>
>> When the time came to re-land that CL again, it didn't change much - I
>> just removed that file from the patch.
>> Was it fine to TBR the people that had reviewed the CL already (I
>> actually did so)?
>
>
> I think this is another case of judgement, and I agree that if files by
> owners didn't change the second time around, no need to ask them to look at
> it again. Of course, someone needs to review the changes from the first
> commit to the second commit.
>

Ok, thank you.

[Nico Weber]

On Wed, Mar 27, 2013 at 8:44 AM, Alexander Potapenko <gli...@chromium.org> wrote:

>
> I'm not sure what occassionally included means?
>

There was a hunk touching some file unrelated to the change, and I've
occasionally included that file into the list of files in the CL when
doing gcl change.

(I think you mean "accidentally")

 

>>
>> When the time came to re-land that CL again, it didn't change much - I
>> just removed that file from the patch.
>> Was it fine to TBR the people that had reviewed the CL already (I
>> actually did so)?
>
>
> I think this is another case of judgement, and I agree that if files by
> owners didn't change the second time around, no need to ask them to look at
> it again. Of course, someone needs to review the changes from the first
> commit to the second commit.
>

Ok, thank you.

[Randy Smith]

On Wed, Mar 27, 2013 at 11:27 AM, John Abd-El-Malek <jabde...@google.com> wrote:

On Wed, Mar 27, 2013 at 1:09 AM, Alexander Potapenko <gli...@chromium.org> wrote:

I've had a CL recently that had been reviewed, committed, but then
reverted because an extra file was occasionally included into that CL.

I'm not sure what occassionally included means?

 

When the time came to re-land that CL again, it didn't change much - I
just removed that file from the patch.
Was it fine to TBR the people that had reviewed the CL already (I
actually did so)?

I think this is another case of judgement, and I agree that if files by owners didn't change the second time around, no need to ask them to look at it again. Of course, someone needs to review the changes from the first commit to the second commit. 

So I think this is parallel to the case where you get LGTMs, and run into a problem in the try bots, and fix that problem, and then go to land.  My attitude to date has been that a re-review in that case is a judgement call--if the changes are big enough, I ask for a re-review, and if they aren't, I don't.  

So I'm not sure I agree with your "Of course"; care to comment on what I'm getting either right or wrong, in your opinion?

-- Randy

[Eric Noyau]

Is it acceptable to land a TBR for code that has been peer-programmed? As long as it doesn't need a owner review or one of the two is an owner of course. Or do rephrase the question, does peer-programming count as a peer review?

-- Eric

[Avi Drissman]

On Wed, Mar 27, 2013 at 11:49 AM, Eric Noyau <no...@chromium.org> wrote:

Is it acceptable to land a TBR for code that has been peer-programmed?

My opinion is no.

The point of a code review is to have a fresh set of eyes not involved in the coding review it to make sure it looks good and you're not missing anything. Having the code reviewer be one of the people who coded it defeats that purpose.

Avi

[John Abd-El-Malek]

On Wed, Mar 27, 2013 at 8:48 AM, Randy Smith <rds...@google.com> wrote:

On Wed, Mar 27, 2013 at 11:27 AM, John Abd-El-Malek <jabde...@google.com> wrote:

On Wed, Mar 27, 2013 at 1:09 AM, Alexander Potapenko <gli...@chromium.org> wrote:

I've had a CL recently that had been reviewed, committed, but then
reverted because an extra file was occasionally included into that CL.

I'm not sure what occassionally included means?

 

When the time came to re-land that CL again, it didn't change much - I
just removed that file from the patch.
Was it fine to TBR the people that had reviewed the CL already (I
actually did so)?

I think this is another case of judgement, and I agree that if files by owners didn't change the second time around, no need to ask them to look at it again. Of course, someone needs to review the changes from the first commit to the second commit. 

So I think this is parallel to the case where you get LGTMs, and run into a problem in the try bots, and fix that problem, and then go to land.  My attitude to date has been that a re-review in that case is a judgement call--if the changes are big enough, I ask for a re-review, and if they aren't, I don't.  

So I'm not sure I agree with your "Of course"; care to comment on what I'm getting either right or wrong, in your opinion?

How is that different from landing a change, unreviewed, of the diff that made the trybots pass?

This is why I try to have green try runs before sending for review. It's hard for me to predict what will fail on the try jobs after a reviewer sent their lgtm.

[Robert Flack]

I assume we can / have to continue to land binary only changes without try jobs as the CQ doesn't handle binary patches correctly yet.

[Matt Menke]

On Wednesday, March 27, 2013 12:04:35 PM UTC-4, John Abd-El-Malek wrote:

On Wed, Mar 27, 2013 at 8:48 AM, Randy Smith <rds...@google.com> wrote:

On Wed, Mar 27, 2013 at 11:27 AM, John Abd-El-Malek <jabde...@google.com> wrote:

On Wed, Mar 27, 2013 at 1:09 AM, Alexander Potapenko <gli...@chromium.org> wrote:

I've had a CL recently that had been reviewed, committed, but then
reverted because an extra file was occasionally included into that CL.

I'm not sure what occassionally included means?

 

When the time came to re-land that CL again, it didn't change much - I
just removed that file from the patch.
Was it fine to TBR the people that had reviewed the CL already (I
actually did so)?

I think this is another case of judgement, and I agree that if files by owners didn't change the second time around, no need to ask them to look at it again. Of course, someone needs to review the changes from the first commit to the second commit. 

So I think this is parallel to the case where you get LGTMs, and run into a problem in the try bots, and fix that problem, and then go to land.  My attitude to date has been that a re-review in that case is a judgement call--if the changes are big enough, I ask for a re-review, and if they aren't, I don't.  

So I'm not sure I agree with your "Of course"; care to comment on what I'm getting either right or wrong, in your opinion?

How is that different from landing a change, unreviewed, of the diff that made the trybots pass?

If you apply that logic to the case of merge conflicts, you suddenly make some CLs very difficult to land.

 

This is why I try to have green try runs before sending for review. It's hard for me to predict what will fail on the try jobs after a reviewer sent their lgtm.

While having a successful run on all platforms is great, some days try runs are extremely flaky.  Yesterday seemed a fairly good day to me, and I still had two trybot LKGR compile failures that had nothing to do with my CLs, and a number of browser test failures that also had nothing to do with my CLs.  The browser test failures you can often reason away (Particularly if they're only on one platform), but unrelated compile failures are a bigger problem.

And on flaky days, 50% or more of runs that do browser tests can have random test failures.  When I was a noogler, I reran tests a lot waiting for green runs, but this sometimes delayed CLs for days, and was just a huge waste of time.  Admittedly, our try runs are faster now, but there are still a great number of days where a green try run is simply not going to happen.

[Brett Wilson]

There is actually an official Google policy on this:
http://www/eng/code_review.html (internal only)

"If you pair programmed to write the code and that other person is
someone to whom you would otherwise have sent the review, the code is
considered reviewed. If you are writing a new subsystem, interface or
API that will be widely used, consider having a third person who was
not in on the original discussions review it."

If you're doing pair programming, the other person should be there and
should be able to give a real "LGTM" on the code review right away.
"TBR" literally means "To Be Reviewed" which doesn't apply and I think
sends the wrong message.

Brett

[Randy Smith]

On Wed, Mar 27, 2013 at 12:04 PM, John Abd-El-Malek <jabde...@google.com> wrote:

On Wed, Mar 27, 2013 at 8:48 AM, Randy Smith <rds...@google.com> wrote:

On Wed, Mar 27, 2013 at 11:27 AM, John Abd-El-Malek <jabde...@google.com> wrote:

On Wed, Mar 27, 2013 at 1:09 AM, Alexander Potapenko <gli...@chromium.org> wrote:

I've had a CL recently that had been reviewed, committed, but then
reverted because an extra file was occasionally included into that CL.

I'm not sure what occassionally included means?

 

When the time came to re-land that CL again, it didn't change much - I
just removed that file from the patch.
Was it fine to TBR the people that had reviewed the CL already (I
actually did so)?

I think this is another case of judgement, and I agree that if files by owners didn't change the second time around, no need to ask them to look at it again. Of course, someone needs to review the changes from the first commit to the second commit. 

So I think this is parallel to the case where you get LGTMs, and run into a problem in the try bots, and fix that problem, and then go to land.  My attitude to date has been that a re-review in that case is a judgement call--if the changes are big enough, I ask for a re-review, and if they aren't, I don't.  

So I'm not sure I agree with your "Of course"; care to comment on what I'm getting either right or wrong, in your opinion?

How is that different from landing a change, unreviewed, of the diff that made the trybots pass?

Reviews happen at several different levels simultaneously.  LGTM simultaneously means: "Yeah, this makes sense to do", "basically the right architecture", "good, clean design", and "no nits that I can see" (and many other things I'm not calling out).  IMO, several (not all) of those carry over for a tweak to get try bots to go green/get past the CQ.  

But at the core, my argument is the same as the one Matt brings up.  If you require that you've gotten an LGTM for *precisely* the diff you're landing, you substantially raise the tax for merging if other CLs conflicting with you go in before yours, and for latency delay for try jobs (because you can never run them in parallel with the review).  And the longer you work on a CL, the more potential merge conflicts, meaning that this can be an exponentially growing tax (I only wish I wasn't speaking from personal experience on CLs that I personally judged *did* require a re-review).  

When I put all this together, I feel like "getting a re-review on tweaks for an already approved CL" should be in the "use best judgement" category.

-- Randy

[John Abd-El-Malek]

On Wed, Mar 27, 2013 at 11:57 AM, Randy Smith <rds...@google.com> wrote:

On Wed, Mar 27, 2013 at 12:04 PM, John Abd-El-Malek <jabde...@google.com> wrote:

On Wed, Mar 27, 2013 at 8:48 AM, Randy Smith <rds...@google.com> wrote:

On Wed, Mar 27, 2013 at 11:27 AM, John Abd-El-Malek <jabde...@google.com> wrote:

On Wed, Mar 27, 2013 at 1:09 AM, Alexander Potapenko <gli...@chromium.org> wrote:

I've had a CL recently that had been reviewed, committed, but then
reverted because an extra file was occasionally included into that CL.

I'm not sure what occassionally included means?

 

When the time came to re-land that CL again, it didn't change much - I
just removed that file from the patch.
Was it fine to TBR the people that had reviewed the CL already (I
actually did so)?

I think this is another case of judgement, and I agree that if files by owners didn't change the second time around, no need to ask them to look at it again. Of course, someone needs to review the changes from the first commit to the second commit. 

So I think this is parallel to the case where you get LGTMs, and run into a problem in the try bots, and fix that problem, and then go to land.  My attitude to date has been that a re-review in that case is a judgement call--if the changes are big enough, I ask for a re-review, and if they aren't, I don't.  

So I'm not sure I agree with your "Of course"; care to comment on what I'm getting either right or wrong, in your opinion?

How is that different from landing a change, unreviewed, of the diff that made the trybots pass?

Reviews happen at several different levels simultaneously.  LGTM simultaneously means: "Yeah, this makes sense to do", "basically the right architecture", "good, clean design", and "no nits that I can see" (and many other things I'm not calling out).  IMO, several (not all) of those carry over for a tweak to get try bots to go green/get past the CQ.  

But at the core, my argument is the same as the one Matt brings up.  If you require that you've gotten an LGTM for *precisely* the diff you're landing, you substantially raise the tax for merging if other CLs conflicting with you go in before yours, and for latency delay for try jobs (because you can never run them in parallel with the review).  And the longer you work on a CL, the more potential merge conflicts, meaning that this can be an exponentially growing tax (I only wish I wasn't speaking from personal experience on CLs that I personally judged *did* require a re-review).  

When I put all this together, I feel like "getting a re-review on tweaks for an already approved CL" should be in the "use best judgement" category.

To be clear, I never said precisely the diff. However I sometimes see changes be sent out, getting reviewed, and something quite different lands. Obviously updating callsites on other platforms etc is easy for people to agree that these don't need to be looked at. But things like updating tests that failed as a result etc need to be reviewed.

[Peter Kasting]

On Tue, Mar 26, 2013 at 1:45 PM, John Abd-El-Malek <j...@chromium.org> wrote:

The only acceptable uses of TBR are:

-fixing a tree breakage (i.e. because a configuration is broken which isn't run on the try server)

-reverts/merges

-TBRing owners of consumers of API changes which has been reviewed by owners of that API

As a chrome/browser/ui OWNER, I explicitly tell people it's OK to TBR me for OWNERs signoff if they're making some purely-mechanical change (e.g. removing an argument from some widely-implemented base class function prototype) that has already gotten proper review from a reviewer.

As a WebKit sheriff, I TBR my WebKit DEPS updates to the other WebKit sheriff.  Changes like "Update DEPS version of WebKit to r12345" are not really reviewable anyway.

I think these are fairly limited cases, but I do think in general they highlight that TBRing can be used for slightly more than what you said.  Maybe I'm splitting hairs though.

PK

[Anthony LaForge]

Given the discussion, I wonder if it would make sense to publish a more formal set of TBR guidelines/best practices on chromium.org.

Kind Regards,

Anthony Laforge
Technical Program Manager
Mountain View, CA

--

[Thiago Farina]

On Wed, Mar 27, 2013 at 8:55 PM, Anthony LaForge <laf...@google.com> wrote:
> Given the discussion, I wonder if it would make sense to publish a more
> formal set of TBR guidelines/best practices on chromium.org.
>

I'd be happy if update include paths (i.e, when we move a file from
one directory to another) and remove unused includes could be added to
it. I guess the fail into the trivial category and are unlikely to
break the tree if landed through CQ.

--
Thiago

[Dmitry Titov]

In the past, I was told by owners of some high-level gypi files to TBR things like adding a new file to gypi, provided the lower-level reviewer approved the change as a whole... 

[Nico Weber]

On Wed, Mar 27, 2013 at 5:16 PM, Dmitry Titov <dim...@chromium.org> wrote:

In the past, I was told by owners of some high-level gypi files to TBR things like adding a new file to gypi, provided the lower-level reviewer approved the change as a whole... 

Yes, chrome/OWNERS says

# If you just remove, rename, or add a file to a non-test gypi file,

# you can land that change TBR. Make sure to still add the people

# you're TBRing to the review thread and to send them email.

I think this falls under the third point in Jam's mail if you squint enough :-)