Enable merging buildbot builds dependent on queue length

Enable merging buildbot builds dependent on queue length

This enhances custom mergeRequests functions with additional
parameters for the number of unclaimed builds in the queue
and the number of already merged builds.

This allows writing more flexible merge strategies, e.g.:
- Avoid huge blame lists during peak hours that stem from
the standard merge-all strategy.
- Avoid huge backlog on bots using the merge-none strategy.

This also adds an example FYI bot from V8 to test the new
feature.

BUG=654749
TEST=Tested on a local master stressed with fake commits.

Committed: https://chromium.googlesource.com/chromium/tools/build/+/34603b7a540ae40089ddce69aecedf2f6783c7ae

[Michael Achenbach, 2016-10-13 09:34:09]

Description was changed from

==========
Enable merging buildbot builds dependent on queue length

BUG=
==========

to

==========
Enable merging buildbot builds dependent on queue length

BUG=654749
==========

[Michael Achenbach, 2016-10-13 09:35:58]

machenbach@chromium.org changed reviewers:
+ tandrii@chromium.org

[Michael Achenbach, 2016-10-13 09:35:59]

PTAL (preliminary) - will add more reviewers if you don't find it too
crazy/hacky/risky.

[Michael Achenbach, 2016-10-13 10:36:13]

https://codereview.chromium.org/2415703004/diff/40001/third_party/buildbot_8_...
File third_party/buildbot_8_4p1/buildbot/process/builder.py (right):

https://codereview.chromium.org/2415703004/diff/40001/third_party/buildbot_8_...
third_party/buildbot_8_4p1/buildbot/process/builder.py:878:
merged_request_objects.append(other_breq_object)
Thinking more: Now that I'm already changing this code, I could also pass
len(merged_request_objects) to the merge function, so that I could remove the
extra storage hack in there.

[Michael Achenbach, 2016-10-13 12:16:28]

PTAL at patch 4

[Michael Achenbach, 2016-10-13 12:21:57]

Description was changed from

==========
Enable merging buildbot builds dependent on queue length

BUG=654749
==========

to

==========
Enable merging buildbot builds dependent on queue length

This enhances custom mergeRequests functions with additional
parameters for the number of unclaimed builds in the queue
and the number of already merged builds.

This allows writing more flexible merge strategies, e.g.:
- Avoid huge blame lists during peak hours that stem from
the standard merge-all strategy.
- Avoid huge backlog on bots using the merge-none strategy.

This also adds an example FYI bot from V8 to test the new
feature.

BUG=654749
==========

[tandrii(chromium), 2016-10-13 13:35:22]

lgtm

https://codereview.chromium.org/2415703004/diff/60001/third_party/buildbot_8_...
File third_party/buildbot_8_4p1/buildbot/process/builder.py (right):

https://codereview.chromium.org/2415703004/diff/60001/third_party/buildbot_8_...
third_party/buildbot_8_4p1/buildbot/process/builder.py:856: # Make sure the
merge-requests function can take the queue length. Old
...queue length and len(merged_requests).

https://codereview.chromium.org/2415703004/diff/60001/third_party/buildbot_8_...
third_party/buildbot_8_4p1/buildbot/process/builder.py:858:
mergeRequests_with_length_fn = lambda a, b, *_ : mergeRequests_fn(a, b)
inspect on bound class functions or on functions with kwargs or *args will not
quite work in all cases as intended. I'd do the usual python way: try and see
what happens.
def wrapper(a,b,c,d):
  try:
    return mergeRequests_fn(a,b)
  except TypeError:
    return mergeRequests_fn(a,b,c,d)

[tandrii(chromium), 2016-10-13 13:35:50]

On 2016/10/13 13:35:22, tandrii(chromium) wrote:
> lgtm
i didn't mean to click lg-tm button.

[Michael Achenbach, 2016-10-13 13:53:35]

PTAL at patch 5

https://codereview.chromium.org/2415703004/diff/60001/third_party/buildbot_8_...
File third_party/buildbot_8_4p1/buildbot/process/builder.py (right):

https://codereview.chromium.org/2415703004/diff/60001/third_party/buildbot_8_...
third_party/buildbot_8_4p1/buildbot/process/builder.py:856: # Make sure the
merge-requests function can take the queue length. Old
On 2016/10/13 13:35:22, tandrii(chromium) wrote:
> ...queue length and len(merged_requests). 

wrote "lengths" now

https://codereview.chromium.org/2415703004/diff/60001/third_party/buildbot_8_...
third_party/buildbot_8_4p1/buildbot/process/builder.py:858:
mergeRequests_with_length_fn = lambda a, b, *_ : mergeRequests_fn(a, b)
On 2016/10/13 13:35:22, tandrii(chromium) wrote:
> inspect on bound class functions or on functions with kwargs or *args will not
> quite work in all cases as intended. I'd do the usual python way: try and see
> what happens.
> def wrapper(a,b,c,d):
>   try:
>     return mergeRequests_fn(a,b)
>   except TypeError:
>     return mergeRequests_fn(a,b,c,d)

Good catch. Now implemented your suggestion to use an extra attribute and got
rid of inspect.

[Michael Achenbach, 2016-10-13 14:08:31]

Description was changed from

==========
Enable merging buildbot builds dependent on queue length

This enhances custom mergeRequests functions with additional
parameters for the number of unclaimed builds in the queue
and the number of already merged builds.

This allows writing more flexible merge strategies, e.g.:
- Avoid huge blame lists during peak hours that stem from
the standard merge-all strategy.
- Avoid huge backlog on bots using the merge-none strategy.

This also adds an example FYI bot from V8 to test the new
feature.

BUG=654749
==========

to

==========
Enable merging buildbot builds dependent on queue length

This enhances custom mergeRequests functions with additional
parameters for the number of unclaimed builds in the queue
and the number of already merged builds.

This allows writing more flexible merge strategies, e.g.:
- Avoid huge blame lists during peak hours that stem from
the standard merge-all strategy.
- Avoid huge backlog on bots using the merge-none strategy.

This also adds an example FYI bot from V8 to test the new
feature.

BUG=654749
TEST=Tested on a local master stressed with fake commits.
==========

[tandrii(chromium), 2016-10-13 14:11:50]

LGTM

[Michael Achenbach, 2016-10-13 14:14:44]

machenbach@chromium.org changed reviewers:
+ agable@chromium.org, maruel@chromium.org, stip@chromium.org

[Michael Achenbach, 2016-10-13 14:14:44]

+ a few buildbot experts

PTAL

[agable, 2016-10-13 17:58:37]

One comment. Also, make sure that you include this diff in the
third_party/buildbot_8_4p1/README.chromium.

https://codereview.chromium.org/2415703004/diff/80001/third_party/buildbot_8_...
File third_party/buildbot_8_4p1/buildbot/process/builder.py (right):

https://codereview.chromium.org/2415703004/diff/80001/third_party/buildbot_8_...
third_party/buildbot_8_4p1/buildbot/process/builder.py:874:
mergeRequests_with_length_fn(
It might be less confusing if the "if fn.with_length" logic were down here, and
then whether you pass two or four arguments is dependent on the result of that
check, rather than always passing four and having to decode the lambda above. Up
to you, though.

[Michael Achenbach, 2016-10-13 19:05:07]

PTAL at patch 6. Didn't test it locally yet, but will do before landing
anything.

On 2016/10/13 17:58:37, agable wrote:
> One comment. Also, make sure that you include this diff in the
> third_party/buildbot_8_4p1/README.chromium.

Looks like that file was made more generic by
https://codereview.chromium.org/1383143003 . Guess there's nothing to do then?

https://codereview.chromium.org/2415703004/diff/80001/third_party/buildbot_8_...
File third_party/buildbot_8_4p1/buildbot/process/builder.py (right):

https://codereview.chromium.org/2415703004/diff/80001/third_party/buildbot_8_...
third_party/buildbot_8_4p1/buildbot/process/builder.py:874:
mergeRequests_with_length_fn(
On 2016/10/13 17:58:36, agable wrote:
> It might be less confusing if the "if fn.with_length" logic were down here,
and
> then whether you pass two or four arguments is dependent on the result of that
> check, rather than always passing four and having to decode the lambda above.
Up
> to you, though.

Done.

[agable, 2016-10-13 20:04:08]

Good catch on the README; lgtm.

[Michael Achenbach, 2016-10-14 07:28:57]

The CQ bit was checked by machenbach@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 07:29:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 07:33:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 07:33:01]

Dry run: This issue passed the CQ dry run.

[Michael Achenbach, 2016-10-14 07:40:21]

CC current trooper. Tested locally again and looks good. Landing this now.

[Michael Achenbach, 2016-10-14 07:40:59]

The CQ bit was checked by machenbach@chromium.org

[Michael Achenbach, 2016-10-14 07:41:00]

The patchset sent to the CQ was uploaded after l-g-t-m from tandrii@chromium.org
Link to the patchset: https://codereview.chromium.org/2415703004/#ps100001
(title: "Review")

[commit-bot: I haz the power, 2016-10-14 07:41:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 07:44:56]

Description was changed from

==========
Enable merging buildbot builds dependent on queue length

This enhances custom mergeRequests functions with additional
parameters for the number of unclaimed builds in the queue
and the number of already merged builds.

This allows writing more flexible merge strategies, e.g.:
- Avoid huge blame lists during peak hours that stem from
the standard merge-all strategy.
- Avoid huge backlog on bots using the merge-none strategy.

This also adds an example FYI bot from V8 to test the new
feature.

BUG=654749
TEST=Tested on a local master stressed with fake commits.
==========

to

==========
Enable merging buildbot builds dependent on queue length

This enhances custom mergeRequests functions with additional
parameters for the number of unclaimed builds in the queue
and the number of already merged builds.

This allows writing more flexible merge strategies, e.g.:
- Avoid huge blame lists during peak hours that stem from
the standard merge-all strategy.
- Avoid huge backlog on bots using the merge-none strategy.

This also adds an example FYI bot from V8 to test the new
feature.

BUG=654749
TEST=Tested on a local master stressed with fake commits.

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/34603b7a540ae40089dd...
==========

[commit-bot: I haz the power, 2016-10-14 07:44:58]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/tools/build/+/34603b7a540ae40089dd...