Include UUID in ActionState

[zbirenbaum]

Description

Changes the field used for identifying which action an ActionState corresponds to include a uuid as well as the ActionInfoHashKey. All existing functionality is kept the same by making use of the nested ActionInfoHashKey contained within the Id. These changes will provide the basis for all usage of Id in followup changes to the scheduler. This breaks compatibility for forwarding an operation from one remote execution system to another that does not use our operation name format (ie: very unlikely, but possible).

Please delete options that aren't relevant.

• Refactor
• Breaking Change

How Has This Been Tested?

All existing tests have been modified to remain functionally equivalent.

Checklist

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

---

This change is 

[CLAassistant]

All committers have signed the CLA.

[adam-singer]

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: license/cla, pre-commit-checks, and 1 discussions need to be resolved

---

nativelink-util/src/action_messages.rs line 60 at r1 (raw file):

}

impl Id {

Can we make units for Id that test creation, comparison, parsing? Would also be nice to get a sense of what this field looks like as a string to be parsed.

[zbirenbaum]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: 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), license/cla, macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 1 discussions need to be resolved

---

nativelink-util/src/action_messages.rs line 60 at r1 (raw file):

Previously, adam-singer (Adam Singer) wrote…

Can we make units for Id that test creation, comparison, parsing? Would also be nice to get a sense of what this field looks like as a string to be parsed.

The format for the string is just {unique_qualifier}:{uuid}

@allada I believe when we just spoke you felt tests weren't required for this refactor as long as all existing tests pass correct?

[allada]

Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), license/cla, macos-13, ubuntu-22.04, and 4 discussions need to be resolved

---

nativelink-scheduler/src/simple_scheduler.rs line 203 at r2 (raw file):

    }
}
impl Borrow<ActionInfoHashKey> for CompletedAction {

I think you can remove this now?

---

nativelink-util/src/action_messages.rs line 60 at r1 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

The format for the string is just {unique_qualifier}:{uuid}

@allada I believe when we just spoke you felt tests weren't required for this refactor as long as all existing tests pass correct?

I'd say yes, we probably don't need a test for this unless we want the external-visible format of OperationId to be stable across versions, but I don't think we should strive for that just yet.

---

nativelink-util/src/action_messages.rs line 103 at r2 (raw file):

impl std::fmt::Debug for OperationId {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        let id_str = format!("{}:{}", self.unique_qualifier.action_name(), self.id);

nit: use f.write_fmt(). It will remove a needless string allocation.

---

nativelink-util/src/action_messages.rs line 112 at r2 (raw file):

pub struct WorkerId(pub Uuid);

impl std::fmt::Display for WorkerId {

ditto.

[zbirenbaum]

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Vercel, pre-commit-checks, and 3 discussions need to be resolved

---

nativelink-scheduler/src/simple_scheduler.rs line 203 at r2 (raw file):

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

I think you can remove this now?

Unfortunately not. Until everything is migrated to use an OperationId map, find_recently_completed_action and probably other places will error out if you remove this code.

---

nativelink-util/src/action_messages.rs line 103 at r2 (raw file):

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

nit: use f.write_fmt(). It will remove a needless string allocation.

Done.

---

nativelink-util/src/action_messages.rs line 112 at r2 (raw file):

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

ditto.

Done.

[zbirenbaum]

Dismissed @adam-singer from a discussion.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04 / stable, windows-2022 / stable, and 2 discussions need to be resolved

---

nativelink-util/src/action_messages.rs line 60 at r1 (raw file):

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

I'd say yes, we probably don't need a test for this unless we want the external-visible format of OperationId to be stable across versions, but I don't think we should strive for that just yet.

Done.

[adam-singer]

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

[allada]

Reviewed 3 of 4 files at r4, all commit messages.
Reviewable status: complete! 2 of 1 LGTMs obtained

[allada]

Reviewed 1 of 9 files at r1, 1 of 4 files at r4.
Reviewable status: complete! 2 of 1 LGTMs obtained