Implementation of ClientStateManager

[adam-singer]

Description

Implementation of ClientStateManager where the beginnings of being able to tuck mutations behind a trait for StateManager. StateManager now wraps the StateManagerImpl for inner state structure StateManagerImpl. All mutations should be done on a lock on inner in the future. Moving implementation code of add_action and filter_operations into ClientStateManager trait. Updated accessor code from state_manager to state_manager.inner.

Fixes #990

Type of change

Please delete options that aren't relevant.

• Refactor

How Has This Been Tested?

Existing tests

Checklist

• bazel test //... passes locally
• PR is contained in a single commit, using git amend see some docs

---

This change is 

[adam-singer]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, and 1 discussions need to be resolved

---

nativelink-scheduler/src/scheduler_state/client_action_state_result.rs line 25 at r1 (raw file):

pub(crate) struct ClientActionStateResult {
    //notify_channel: Receiver<Arc<ActionState>>,

remove

[allada]

Looks good.

Reviewed 3 of 7 files at r1.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 7 discussions need to be resolved

---

nativelink-scheduler/src/scheduler_state/client_action_state_result.rs line 31 at r1 (raw file):

impl ClientActionStateResult {
    pub(crate) fn new(mut rx: Receiver<Arc<ActionState>>) -> Self {
        rx.mark_changed();

nit: Add comment on why we are marking it changed.

---

nativelink-scheduler/src/scheduler_state/state_manager.rs line 185 at r1 (raw file):

        filter: OperationFilter,
    ) -> Result<ActionStateResultStream, Error> {
        let unique_qualifier = &filter

nit: Add todo to build this out to a proper filter.

---

nativelink-scheduler/src/scheduler_state/state_manager.rs line 194 at r1 (raw file):

            .and_then(|action_info| self.inner.queued_actions.get(action_info))
            .or_else(|| self.inner.active_actions.get(unique_qualifier));
        let client_action_state_result: Pin<Box<Iter<IntoIter<Arc<dyn ActionStateResult>>>>> =

nit: Can we remove this type? Forcing the compiler down a specific type route reduces possible optimizations.

---

nativelink-scheduler/src/scheduler_state/state_manager.rs line 197 at r1 (raw file):

            Box::pin(
                awaited_action
                    .map(|aa| {

aa?

---

nativelink-scheduler/src/scheduler_state/state_manager.rs line 200 at r1 (raw file):

                        let rx = aa.notify_channel.subscribe();
                        let action_result: Vec<Arc<dyn ActionStateResult>> =
                            vec![Arc::new(ClientActionStateResult::new(rx))];

nit: Use a fixed array instead, no need to make a heap allocation.

---

nativelink-scheduler/src/scheduler_state/state_manager.rs line 203 at r1 (raw file):

                        stream::iter(action_result)
                    })
                    .unwrap_or_else(|| stream::iter(Vec::new())),

nit: ditto.

[adam-singer]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 3 discussions need to be resolved

---

nativelink-scheduler/src/scheduler_state/client_action_state_result.rs line 31 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: Add comment on why we are marking it changed.

Done.

---

nativelink-scheduler/src/scheduler_state/state_manager.rs line 185 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: Add todo to build this out to a proper filter.

Done.

---

nativelink-scheduler/src/scheduler_state/state_manager.rs line 194 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: Can we remove this type? Forcing the compiler down a specific type route reduces possible optimizations.

Done.

---

nativelink-scheduler/src/scheduler_state/state_manager.rs line 197 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

aa?

Done.

---

nativelink-scheduler/src/scheduler_state/state_manager.rs line 200 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: Use a fixed array instead, no need to make a heap allocation.

Seems like some error between fixed size vec and stream::iter where the size is encoded into the type signature

[adam-singer]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Vercel, asan / ubuntu-22.04, integration-tests (20.04), pre-commit-checks, ubuntu-22.04, vale, and 1 discussions need to be resolved

---

nativelink-scheduler/src/scheduler_state/state_manager.rs line 200 at r1 (raw file):

Previously, adam-singer (Adam Singer) wrote…

Seems like some error between fixed size vec and stream::iter where the size is encoded into the type signature

Done.

---

nativelink-scheduler/src/scheduler_state/state_manager.rs line 203 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: ditto.

Done.

[allada]

Reviewed 2 of 7 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 of 1 LGTMs obtained

[allada]

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and 1 discussions need to be resolved

---

-- commits line 2 at r4:
nit: missing title.

[adam-singer]

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), pre-commit-checks, ubuntu-20.04 / stable, vale

---

-- commits line 2 at r4:

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: missing title.

Done.

[allada]

Reviewed all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Publish image, Publish nativelink-worker-lre-cc