Add OrderBy field for OperationFilter

[adam-singer]

Description

Adjustment to operation state manager api to include a order by field.

Fixes # #983

Type of change

• Refactor (non-breaking change which no functionality added)

How Has This Been Tested?

Please also list any relevant details for your test configuration

Checklist

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

---

This change is 

[allada]

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

---

nativelink-scheduler/src/operation_state_manager.rs line 91 at r1 (raw file):

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct OrderBy {
    pub fields: Vec<OperationFields>,

nit: Public structs need all fields documented.

---

nativelink-scheduler/src/operation_state_manager.rs line 101 at r1 (raw file):

    /// Add a new action to the queue or joins an existing action.
    async fn add_action(
        &mut self,

nit: We don't want to require mut. Lets use internal locking for places that need exclusive access.

Exclusiveness implies it cannot be in an Arc or have any other owners. I'd rather abstract this away behind the interface, like stores do.

[zbirenbaum]

Reviewable status: 1 of 1 LGTMs obtained, and 2 discussions need to be resolved

---

nativelink-scheduler/src/operation_state_manager.rs line 91 at r1 (raw file):

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

nit: Public structs need all fields documented.

I'm not sure how we would choose to sort by descending or ascending here, and I think that something like last_worker_update might be relevant as the timestamp in question rather than just insert timestamp.

If we have a specific use case in mind I don't mind this as is, just not sure the use case for sorting by timestamp.

[adam-singer]

Reviewable status: 1 of 1 LGTMs obtained, and 2 discussions need to be resolved

---

nativelink-scheduler/src/operation_state_manager.rs line 91 at r1 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

I'm not sure how we would choose to sort by descending or ascending here, and I think that something like last_worker_update might be relevant as the timestamp in question rather than just insert timestamp.

If we have a specific use case in mind I don't mind this as is, just not sure the use case for sorting by timestamp.

I imagine one reason could be latent actions based on timestamps that have no laster worker update?

---

nativelink-scheduler/src/operation_state_manager.rs line 101 at r1 (raw file):

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

nit: We don't want to require mut. Lets use internal locking for places that need exclusive access.

Exclusiveness implies it cannot be in an Arc or have any other owners. I'd rather abstract this away behind the interface, like stores do.

sg

[adam-singer]

Reviewable status: 1 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

---

nativelink-scheduler/src/operation_state_manager.rs line 91 at r1 (raw file):

Previously, adam-singer (Adam Singer) wrote…

I imagine one reason could be latent actions based on timestamps that have no laster worker update?

Done.

---

nativelink-scheduler/src/operation_state_manager.rs line 101 at r1 (raw file):

Previously, adam-singer (Adam Singer) wrote…

sg

Done.

[allada]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 1 LGTMs obtained