-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Send batch requests when scheduling new commit builds #1999
Conversation
@@ -132,6 +134,9 @@ class FakeConfig implements Config { | |||
@override | |||
int get schedulingShardSize => 5; | |||
|
|||
@override | |||
int get batchSize => batchSizeValue ?? 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making this const in config.dart would prevent need to add this stub in the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to have customized batchSize
values to validate from test.
await _uploadToBigQuery(commit); | ||
} | ||
|
||
/// Schedule all builds in batch requests instead of a single request. | ||
/// | ||
/// Each batch request contains `kBatchSize` builds to be scheduled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Each batch request contains `kBatchSize` builds to be scheduled. | |
/// Each batch request contains [Config.batchSize] builds to be scheduled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
for (int i = 0; i < toBeScheduled.length; i += config.batchSize) { | ||
futures.add(luciBuildService.schedulePostsubmitBuilds( | ||
commit: commit, | ||
toBeScheduled: toBeScheduled.sublist(i, min(i + config.batchSize, toBeScheduled.length)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is sublist upper bound index non-inclusive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
With cocoon scheduling, a single schedule request of all builds will be created. This causes unpredicted builds schedule failure.
This PR use batch requests instead of a single request of ~300 builds: each batch is with default 5 builds.
Validation: a test version of cocoon wit this change has been running since yesterday.
Related: flutter/flutter#103443, flutter/flutter#92300