-
Notifications
You must be signed in to change notification settings - Fork 125
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
Prepare scheduler config & move owner of notify task change owner #1306
Prepare scheduler config & move owner of notify task change owner #1306
Conversation
faa0a7a
to
8489ca1
Compare
8489ca1
to
b774be5
Compare
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.
+@aaronmondal (please only review r1:r2), this PR is dependent on https://reviewable.io/reviews/TraceMachina/nativelink/1305
Reviewable status: 0 of 1 LGTMs obtained, and 0 of 6 files reviewed, and pending CI: pre-commit-checks (waiting on @aaronmondal)
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.
Reviewable status: 0 of 1 LGTMs obtained, and 0 of 6 files reviewed, and pending CI: Analyze (javascript-typescript), Analyze (python), Local / ubuntu-22.04, NativeLink.com Cloud / Remote Cache (Legacy Dockerfile Test), Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (22.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable (waiting on @aaronmondal)
nativelink-scheduler/src/default_scheduler_factory.rs
line 92 at r2 (raw file):
.unwrap_or(&ExperimentalSimpleSchedulerBackend::memory) { ExperimentalSimpleSchedulerBackend::memory => {
Note: We will soon be adding :redis
here, so it was restructured this way to support the changes about to come.
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.
or: +@adam-singer (if you want to do these two)
Reviewable status: 0 of 2 LGTMs obtained, and 0 of 6 files reviewed, and pending CI: Analyze (javascript-typescript), Analyze (python), Local / ubuntu-22.04, NativeLink.com Cloud / Remote Cache (Legacy Dockerfile Test), Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (22.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable (waiting on @aaronmondal and @adam-singer)
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.
Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained, and all files reviewed (waiting on @aaronmondal)
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.
Reviewable status: 1 of 2 LGTMs obtained, and all files reviewed (waiting on @aaronmondal)
It is easier to do this in one PR than in two because they are dependent on each other. 1. Moves where notifications happen in the scheduler to happen instead in the underlying AwaitedActionDb. 2. Prepare the config changes (non-breaking). towards TraceMachina#359
b774be5
to
627d6c9
Compare
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.
Reviewable status: complete! 1 of 1 LGTMs obtained, and all files reviewed
It is easier to do this in one PR than in two because they are dependent
on each other.
in the underlying AwaitedActionDb.
towards #359
This change is