-
Notifications
You must be signed in to change notification settings - Fork 98
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
[scheduler] Add backfilling #1776
Conversation
app_dart/lib/src/service/config.dart
Outdated
@@ -46,7 +46,7 @@ class Config { | |||
/// This adds support for the `waiting for tree to go green label` to the repo. | |||
/// | |||
/// Relies on the GitHub Checks API being enabled for this repo. | |||
static Set<RepositorySlug> supportedRepos = <RepositorySlug>{ | |||
Set<RepositorySlug> supportedRepos = <RepositorySlug>{ |
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.
This needed to be changed so tests could override it. Otherwise, the data would look weird where there would be 6 backfills instead of 1 (1 for each supported repo)
return null; | ||
} | ||
|
||
final List<FullTask> backfillTask = |
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.
It would be interesting to use this logic to rerun failed tests to surface flakes. Not needed in this PR though.
late FakePubSub pubsub; | ||
late FakeScheduler scheduler; | ||
|
||
group('BatchBackfiller', () { |
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.
could you add tests for batch scheduling cases: more than 1 target are scheduled when they all need to schedule; only one target is scheduled when itself needs to schedule, etc?
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.
Thanks, good point on having multiple columns being tested at a time.
To clarify, I added tests for when:
- Only one column needs to be backfilled
- 2 columns need to be backfilled
@@ -27,6 +27,10 @@ cron: | |||
url: /api/vacuum-github-commits | |||
schedule: every 1 hours | |||
|
|||
- description: backfills builds | |||
url: /api/scheduler/batch-backfiller | |||
schedule: every 5 minutes |
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 believe this is a no-op for LUCI triggered builders?
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.
Correct. Those targets have a OmitPolicy which means Cocoon doesn't do anything.
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
flutter/flutter#100793
This implements a cron job that looks at task columns, and backfills them. It will only run a max of one task per column at a given time.
This is needed to migrate the devicelab tests over to the cocoon scheduler.