Create scheduler state module #968
Conversation
adam-singer
force-pushed
the
adams/create-scheduler-state-manager
branch
2 times, most recently
from
7c21633 to
7736560
Compare
There was a problem hiding this comment.
I love this PR!
Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and 5 discussions need to be resolved (waiting on @adam-singer)
nativelink-scheduler/src/scheduler_state/awaited_action.rs line 26 at r1 (raw file):
/// An action that is being awaited on and last known state. pub struct AwaitedAction { pub(crate) action_info: Arc<ActionInfo>,
nit: Lets document these.
nativelink-scheduler/src/scheduler_state/completed_action.rs line 24 at r1 (raw file):
pub struct CompletedAction { pub(crate) completed_time: SystemTime,
nit: ditto.
nativelink-scheduler/src/scheduler_state/manager.rs line 25 at r1 (raw file):
use crate::scheduler_state::workers::Workers; pub(crate) struct Manager {
nit: Lets just name this StateManager.
nativelink-scheduler/src/scheduler_state/manager.rs line 36 at r1 (raw file):
// Important: These two fields must be kept in-sync, so if you modify one, you likely need to // modify the other. pub(crate) queued_actions_set: HashSet<Arc<ActionInfo>>,
nit: ditto.
nativelink-scheduler/src/scheduler_state/workers.rs line 24 at r1 (raw file):
use crate::worker::{Worker, WorkerId, WorkerTimestamp}; pub struct Workers {
nit: ditto.
There was a problem hiding this comment.
This looks great! Super clean - really like the crate level visibility, definitely something that I'll want to include on my side. I think the comment nits Blaise left are the only real things to address.
Reviewable status: 0 of 1 LGTMs obtained, and 5 discussions need to be resolved (waiting on @adam-singer)
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained, and 1 discussions need to be resolved
nativelink-scheduler/src/scheduler_state/awaited_action.rs line 26 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: Lets document these.
Done.
nativelink-scheduler/src/scheduler_state/completed_action.rs line 24 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: ditto.
Done.
nativelink-scheduler/src/scheduler_state/manager.rs line 25 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: Lets just name this
StateManager.
Done.
nativelink-scheduler/src/scheduler_state/manager.rs line 36 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: ditto.
Done.
adam-singer
force-pushed
the
adams/create-scheduler-state-manager
branch
from
7736560 to
c17bf9b
Compare
Moving enough code out of `simple_scheduler` for decoupling stateful structures to support trait based interfaces in the future. There is no logical flow change in this refactor, all tests pass. Some structures will have `pub(crate)` visable modules and fields to help with the transition of code reordering.
adam-singer
force-pushed
the
adams/create-scheduler-state-manager
branch
from
c17bf9b to
5334be4
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Vercel, docker-compose-compiles-nativelink (20.04), pre-commit-checks, ubuntu-20.04 / stable, vale (waiting on @adam-singer)
nativelink-scheduler/src/scheduler_state/workers.rs line 24 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: ditto.
Done.
Description
Moving enough code out of
simple_schedulerfor decoupling stateful structures to support trait based interfaces in the future. There is no logical flow change in this refactor, all tests pass. Some structures will havepub(crate)visable modules and fields to help with the transition of code reordering.Fixes # #982
Type of change
How Has This Been Tested?
Checklist
bazel test //...passes locallygit amendsee some docsThis change is