Sort issues by their age in the commit queue.

Sort issues by their age in the commit queue.

Previously, CQ was processing issues sorted by issue ID, which may result in processing a very recent CL before a very old CL. The age is based on when the Commit box was last checked (and not when the CL was first created).

This will hopefully help to clear the oldest CLs first, improve patch success rate, and keep the CQ's maximum age down.

BUG=335187
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=247767

[Paweł Hajdan Jr., 2014-01-29 01:12:38]

https://codereview.chromium.org/145293006/diff/1/pending_manager.py
File pending_manager.py (right):

https://codereview.chromium.org/145293006/diff/1/pending_manager.py#newcode24
pending_manager.py:24: import datetime
nit: Please sort imports alphabetically.

https://codereview.chromium.org/145293006/diff/1/pending_manager.py#newcode53
pending_manager.py:53: sort_key = unicode
I don't think the order of these fields is meaningful for semantics, but putting
it here makes the comment above confusing.

Could you put sort_key maybe in the "Only a cache" section?

https://codereview.chromium.org/145293006/diff/1/pending_manager.py#newcode179
pending_manager.py:179: @staticmethod
Looking at the body of this function it seems it shouldn't be a method at all:
why not just pull it out to be a top-level function?

https://codereview.chromium.org/145293006/diff/1/pending_manager.py#newcode187
pending_manager.py:187: if (m['sender'] == 'commit-bot@chromium.org'
This should match the --user from commit_queue.py

https://codereview.chromium.org/145293006/diff/1/pending_manager.py#newcode189
pending_manager.py:189: issue_data['sort_key'] = m['date']
What type is this? Sorting strings would yield different than expected results.

[Sergey Berezin, 2014-01-29 01:54:15]

I'm fixing most of your comments, except for those below. Next patch is to
follow shortly.

Thanks!

https://codereview.chromium.org/145293006/diff/1/pending_manager.py
File pending_manager.py (right):

https://codereview.chromium.org/145293006/diff/1/pending_manager.py#newcode179
pending_manager.py:179: @staticmethod
Since it needs to access the 'user' option, I'm making it a true method.

On 2014/01/29 01:12:38, Paweł Hajdan Jr. wrote:
> Looking at the body of this function it seems it shouldn't be a method at all:
> why not just pull it out to be a top-level function?

https://codereview.chromium.org/145293006/diff/1/pending_manager.py#newcode189
pending_manager.py:189: issue_data['sort_key'] = m['date']
It is a string in the form 'yyyy-mm-dd hh:mm:ss.dddd' which seems to sort as
expected. I could make it a datetime object, but then I'll have to parse the
'date' string from rietveld. Is it worth it?

On 2014/01/29 01:12:38, Paweł Hajdan Jr. wrote:
> What type is this? Sorting strings would yield different than expected
results.

[Sergey Berezin, 2014-01-29 02:08:47]

Uploaded another patch

[Paweł Hajdan Jr., 2014-01-29 02:38:23]

Sorry about another round, just noticed a way to make it even simpler.

https://codereview.chromium.org/145293006/diff/20001/context.py
File context.py (right):

https://codereview.chromium.org/145293006/diff/20001/context.py#newcode15
context.py:15: user: The --user option to commit_queue
Oh, since this also has Rietveld, does using just rietveld.email work?

If it doesn't, how about changing the documentation of the user argument to say
"Rietveld user" instead?

https://codereview.chromium.org/145293006/diff/20001/pending_manager.py
File pending_manager.py (right):

https://codereview.chromium.org/145293006/diff/20001/pending_manager.py#newco...
pending_manager.py:188: and 'CQ is trying da patch.' in m['text']):
nit: Put the "and" at the end of the previous line.

[Sergey Berezin, 2014-01-29 17:41:42]

Patch #3

https://codereview.chromium.org/145293006/diff/20001/context.py
File context.py (right):

https://codereview.chromium.org/145293006/diff/20001/context.py#newcode15
context.py:15: user: The --user option to commit_queue
Actually, in a dry run, Rietveld receives an empty email (to signal "no
authentication"), while commit queue still has the specified (or default) user
name. We can discuss if this discrepancy should be eliminated later, but for now
I can't rely on rietveld.email.

On 2014/01/29 02:38:24, Paweł Hajdan Jr. wrote:
> Oh, since this also has Rietveld, does using just rietveld.email work?
> 
> If it doesn't, how about changing the documentation of the user argument to
say
> "Rietveld user" instead?

https://codereview.chromium.org/145293006/diff/20001/pending_manager.py
File pending_manager.py (right):

https://codereview.chromium.org/145293006/diff/20001/pending_manager.py#newco...
pending_manager.py:188: and 'CQ is trying da patch.' in m['text']):
On 2014/01/29 02:38:24, Paweł Hajdan Jr. wrote:
> nit: Put the "and" at the end of the previous line.

Done.

[Paweł Hajdan Jr., 2014-01-29 18:49:46]

https://codereview.chromium.org/145293006/diff/20001/context.py
File context.py (right):

https://codereview.chromium.org/145293006/diff/20001/context.py#newcode15
context.py:15: user: The --user option to commit_queue
On 2014/01/29 17:41:43, Sergey Berezin wrote:
> Actually, in a dry run, Rietveld receives an empty email (to signal "no
> authentication"), while commit queue still has the specified (or default) user
> name. We can discuss if this discrepancy should be eliminated later, but for
now
> I can't rely on rietveld.email.

Will it break in dry run? I think that unless it'd be broken it's fine - using
rietveld.email would be simpler it seems.

[Sergey Berezin, 2014-01-29 21:40:48]

Reverted all the changes from other files, and used rietveld.email whenever
possible. I've also hard-coded the default value (e.g. when rietveld object is
missing).

This 'user' hack should go away once Rietveld can tell us about the commit box
natively.

[Paweł Hajdan Jr., 2014-01-29 22:44:59]

LGTM!

[commit-bot: I haz the power, 2014-01-29 22:51:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyberezin@chromium.org/145293006/6...

[commit-bot: I haz the power, 2014-01-29 22:52:11]

Change committed as 247767