Bug 29274 – commit-queue should attribute committers in ChangeLog entries

• Status: NEW
• Component: Tools / Tests

[Eric Seidel (no email)]

commit-queue should attribute committers in ChangeLog entries

Since we have no way (yet?) of making commits come from the person who actually sets the commit-queue flag, we should at least mark up the ChangeLogs to note who set the flag.

Something like:

Reviewed by Darin Adler.
Commit approved by Darin Adler.

Thoughts?

[Mark Rowe (bdash)]

How is approving a commit different than reviewing the patch?

[Eric Seidel (no email)]

Currently the flags are separate in bugzilla.  review+ and commit-queue+.  Maybe I need better wording than "Commit approved".  The goal was to distinguish that even though the patch was committed by the account [e-mail], I may not have actually read the patch in question, or ever seen the bug (as is happening more and more as others use the commit queue).  Ideally patches wouldn't be landed under my svn account credentials.  This bug was one proposal or a "work-around" for the current situation.

[Mark Rowe (bdash)]

The thing I don't understand with that system is who is responsible for cleaning up after a patch that breaks tests or builds?  In the current system that is the committer.  In the system you're proposing the person putting "commit-queue+" on the patch has no knowledge of when the patch will be landed.  That puts the responsibility on the person running the commit-queue.  That means whomever that is needs to be around and keeping an eye on things as patches are landed.  They're the responsible person.  The person setting the "commit-queue+" flag in Bugzilla is not.

[Eric Seidel (no email)]

(In reply to comment #3)
> The thing I don't understand with that system is who is responsible for
> cleaning up after a patch that breaks tests or builds?  In the current system
> that is the committer.  In the system you're proposing the person putting
> "commit-queue+" on the patch has no knowledge of when the patch will be landed.
>  That puts the responsibility on the person running the commit-queue.  That
> means whomever that is needs to be around and keeping an eye on things as
> patches are landed.  They're the responsible person.  The person setting the
> "commit-queue+" flag in Bugzilla is not.

It is true, the delay does warrant concern.  IIRC current policy is that the contributer of the patch is responsible for regressions regardless of when or by whom the patch lands.  At least that's what http://webkit.org/coding/contributing.html seems to say.

Two imperfect answers, regarding the delay jeopardizing tree greenness:

1.  The current commit-queue guarantees that the bots for whatever OS the queue is being run from (currently Mac Leopard) will never break.  It runs the tests, just as the bots do, before committing.  In some future more-perfect world, it could use try-bot results to make breaks of other platforms even less common.

2.  Rollouts.  The commit-queue pauses when the bots go red.  It checks the bot health before every commit.  I would recommend that if the commit approver is not around to clean up after breaks that we just roll regressions out.  "bugzilla-tool rollout" makes rollouts rather easy.


Furthermore, if the bots are green, the commit-queue lands within 15 minutes of cq+, so generally people are still around.

Again, none of those are perfect solutions, but they do lessen my own personal worries about current/future usage of automated commits.

As an aside: Given how many committers currently break the mac build on a regular basis (presumably because they don't run the layout tests locally?) I would bet over time that even our current lame commit-queue would have a better track record than humans as far as keeping the Leopard build green. ;)


Hopefully those thoughts are useful.  I would be interested if you have further thoughts on the issue.

[Mark Rowe (bdash)]

(In reply to comment #4)
> (In reply to comment #3)
> > The thing I don't understand with that system is who is responsible for
> > cleaning up after a patch that breaks tests or builds?  In the current system
> > that is the committer.  In the system you're proposing the person putting
> > "commit-queue+" on the patch has no knowledge of when the patch will be landed.
> >  That puts the responsibility on the person running the commit-queue.  That
> > means whomever that is needs to be around and keeping an eye on things as
> > patches are landed.  They're the responsible person.  The person setting the
> > "commit-queue+" flag in Bugzilla is not.
> 
> It is true, the delay does warrant concern.  IIRC current policy is that the
> contributer of the patch is responsible for regressions regardless of when or
> by whom the patch lands.  At least that's what
> http://webkit.org/coding/contributing.html seems to say.

If I land a patch from a contributor and it breaks a build or introduces a regression, it is my responsibility as the committer to deal with those.  That may just involve making the contributor aware of the issue.  However, if the contributor is not present then it would be irresponsible of me to just sit back and leave the builds broken, tests failing or whatever.  I could attempt to fix them myself.  I could roll the patch out.  

> Two imperfect answers, regarding the delay jeopardizing tree greenness:
> 
> 1.  The current commit-queue guarantees that the bots for whatever OS the queue
> is being run from (currently Mac Leopard) will never break.  It runs the tests,
> just as the bots do, before committing.  In some future more-perfect world, it
> could use try-bot results to make breaks of other platforms even less common.

You're being overly optimistic.

Building and running tests takes time.  There's ample opportunity for another committer to land a patch that would cause the patch being tested to unexpectedly break.  This could of course be addressed by having it spin and rebuild and retest if it detects another commit, but I suspect that is not how things work at present.

You're also only testing a single platform.  Try-bots would help some here, but they still don't guarantee things won't be broken.

> 2.  Rollouts.  The commit-queue pauses when the bots go red.  It checks the bot
> health before every commit.  I would recommend that if the commit approver is
> not around to clean up after breaks that we just roll regressions out. 
> "bugzilla-tool rollout" makes rollouts rather easy.

This still requires someone present to take responsibility for cleaning up the mess.  In our current process the responsible person is the committer.  You haven't elaborated on who you think it is in your process.

> Again, none of those are perfect solutions, but they do lessen my own personal
> worries about current/future usage of automated commits.
> 
> As an aside: Given how many committers currently break the mac build on a
> regular basis (presumably because they don't run the layout tests locally?) I
> would bet over time that even our current lame commit-queue would have a better
> track record than humans as far as keeping the Leopard build green. ;)

The Mac build gets broken by contributors working on Mac OS X much less frequently than non-Mac builds get broken.  In general the Mac build is broken very infrequently and is fixed very quickly, so I'm not really sure what you're saying here.

[Eric Seidel (no email)]

I think we've gone off track from this original bug.  We should probably pick this discussion up on webkit-dev if you're interested in discussing the commit-queue further.

(In reply to comment #5)
> However, if the
> contributor is not present then it would be irresponsible of me to just sit
> back and leave the builds broken, tests failing or whatever.  I could attempt
> to fix them myself.  I could roll the patch out.  

It would be possible to teach a queue to watch the bots and roll out any commit it made if bots started failing.

> This could of course be addressed by having it spin and
> rebuild and retest if it detects another commit, but I suspect that is not how
> things work at present.

This is close to how the queue works today.  If a commit happens between when the patch is built and when it lands, the commit will fail (due to ChangeLogs) and the patch cq-'d.  (see bug 28316)  I have a pending change to make it spin instead of cq- the patch.  I could easily make it more aggressive to spin if tip of tree revision does not match the current checkout.

> You're also only testing a single platform.  Try-bots would help some here, but
> they still don't guarantee things won't be broken.

That's correct.

> This still requires someone present to take responsibility for cleaning up the
> mess.  In our current process the responsible person is the committer.  You
> haven't elaborated on who you think it is in your process.

It is true, the queue does not know how to do rollouts.  Nor do the build-bots.   We could teach the queue how to.

> The Mac build gets broken by contributors working on Mac OS X much less
> frequently than non-Mac builds get broken.  In general the Mac build is broken
> very infrequently and is fixed very quickly, so I'm not really sure what you're
> saying here.

I think you're being overly optimistic. :)

I can't fix the behavior of committers who often break the tree and rarely look at the bots. :)  I can fix a software program if it's breaking the tree. :)