Bug 33395 – Commit bot should land patches in order of last modification date

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Tools / Tests

[Steve Block]

Commit bot should land patches in order of the corresponding bug's last modification date.

This prevents the case where a bug is repeatedly pushed back down the commit queue by other more recently modified bugs.

[Steve Block]

Created attachment 46268 [details]
Patch 1 for Bug 33395

[Eric Seidel (no email)]

Comment on attachment 46268 [details]
Patch 1 for Bug 33395

I think this is a fantastic idea.  However, this will land them in bug modification order, not patch modification order.  It that what you really intended?  I think  the proper fix involves parsing the flag set date out of the attachment and then sorting by that in CommitQueue.next_work_item

I'm about to land a patch which moves CommitQueue.next_work_item away from using fetch_patches_in_commit_queue because of bug 33638.  But we can still easily sort the patches after that change.

r- because this patch doesn't actually do what it claims to do.

[Eric Seidel (no email)]

I had figured you were trying to make it possible for users to commit-queue bugs in a certain order, but now I see you're attempting this as a performance optimization for the queue?  The queue lag times will go down signifigantly as we make the tree more consistently red.  The commit-queue delays are almost entirely due to flakey tests causing the tree to turn red.

[Eric Seidel (no email)]

When I said "consistently red" above, of course I meant "consistently green".  The tree is already pretty consistently red which is the problem! :)

[Steve Block]

(In reply to comment #2)
> (From update of attachment 46268 [details])
> I think this is a fantastic idea.  However, this will land them in bug
> modification order, not patch modification order.  It that what you really
> intended? I think  the proper fix involves parsing the flag set date out of
> the attachment and then sorting by that in CommitQueue.next_work_item
Yes, I think that ideally, we'd land patches in the order in which cq+ was set on the patch. However, wasn't aware that this can be extracted from the bug. Sorting by bug last modification date seemed like a good approximation to this, as most bugs use only a single patch and the setting of the cq+ flag is usually the most recent action. So the patch does what I intended, but I agree this is not the optimal solution.

> I'm about to land a patch which moves CommitQueue.next_work_item away from
> using fetch_patches_in_commit_queue because of bug 33638.  But we can still
> easily sort the patches after that change.
OK, I'll look into extracting the cq+ set date.

> I had figured you were trying to make it possible for users to commit-queue
> bugs in a certain order, but now I see you're attempting this as a performance
> optimization for the queue?
No, this wasn't intended as a performance improvement. Just to make sure that once a bug has been added to the commit queue, it doesn't get pushed down the queue by bugs added later.

[Eric Seidel (no email)]

Interesting the bug page with &ctype=xml does not seem to expose the date/time that the flag setter set the flag.  We could parse that from https://bugs.webkit.org/show_activity.cgi?id=33395, but your initial approximation of bug-last-modify is probably close enough.  It would be better to make the sort behavior explicit, but your initial change is probably sufficient.

[Steve Block]

(In reply to comment #6)
> Interesting the bug page with &ctype=xml does not seem to expose the date/time
> that the flag setter set the flag.  We could parse that from
> https://bugs.webkit.org/show_activity.cgi?id=33395, but your initial
> approximation of bug-last-modify is probably close enough.  It would be better
> to make the sort behavior explicit, but your initial change is probably
> sufficient.
OK, I'll look into extracting the cq+ date from the page. Are there any other examples of parsing the buganizer page I could take a look at?

In the mean time, can you r+ my original patch?

[Adam Barth]

> OK, I'll look into extracting the cq+ date from the page. Are there any other
> examples of parsing the buganizer page I could take a look at?

There are a bunch of unit tests that call the parsing routines.  You can see how they work by starting in bugzilla_unittest.py

[Adam Barth]

Btw, apologies for the state of bugzilla.py.  It was one of our first classes and it's in the middle of slowly being refactored.

[Eric Seidel (no email)]

Comment on attachment 46268 [details]
Patch 1 for Bug 33395

This is better than what we do now, yes.

[Steve Block]

Landed as http://trac.webkit.org/changeset/53542
This sorts the commit queue in order of bug last modification date

Leaving bug open as this fix is only an approximation to the desired behavior, as discussed.

[Eric Seidel (no email)]

Comment on attachment 46268 [details]
Patch 1 for Bug 33395

Obsoleting patch since this was landed.

[Eric Seidel (no email)]

Fixed as part of bug 39519.

*** This bug has been marked as a duplicate of bug 39519 ***