Use StoreKey internally for FilesystemStore

[jhpratt]

Closes #1108

Description

From #1108:

Inside FilesystemStore we always turn the StoreKey into a digest, but in reality it should be using .as_str() for it's final destination storage name. This will make it much more usable for other future things (like BEP).

Type of change

Please delete options that aren't relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
• This change requires a documentation update

How Has This Been Tested?

All existing tests pass without modification.

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 

[MarcusSorealheis]

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-init, 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, windows-2022 / stable, and 1 discussions need to be resolved

---

nativelink-store/src/filesystem_store.rs line 671 at r1 (raw file):

        // Take ownership to avoid lifetime issues.
        // FIXME(jhpratt) Get rid of this eventually.

nit: can you change this not into a TODO rather than a FIXME so that is in line with the other similar notes and get picked up by the relevant tools.

[jhpratt]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), asan / ubuntu-22.04, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04 / stable, vale, windows-2022 / stable

---

nativelink-store/src/filesystem_store.rs line 671 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

nit: can you change this not into a TODO rather than a FIXME so that is in line with the other similar notes and get picked up by the relevant tools.

done

[allada]

Reviewed all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and 5 discussions need to be resolved

a discussion (no related file):
The point of this PR is to enable filtering by list/range, by going into a DigestInfo we are loosing the key's range information making the call pointless.

We are also going to need to add some encoding to the name/format of the files. We probably want to prefix all files with h for digests and s for strings, then when we load them, we can turn them back to the StoreKey enum.

Note: When doing this make sure if it doesn't start with h or s to load them in as DigestInfo for reverse compatibility.

---

---

nativelink-store/src/filesystem_store.rs line 78 at r2 (raw file):

    shared_context: Arc<SharedContext>,
    path_type: PathType,
    digest: FileNameDigest,

This should be a StoreKey<'a>

---

nativelink-store/src/filesystem_store.rs line 671 at r2 (raw file):

        // Take ownership to avoid lifetime issues.
        // TODO(jhpratt) Get rid of this eventually.

nit: Lets remove this comment unless there's a ticket, this will be just noise.

---

nativelink-store/src/filesystem_store.rs line 672 at r2 (raw file):

        // Take ownership to avoid lifetime issues.
        // TODO(jhpratt) Get rid of this eventually.
        let final_key = final_key.borrow().into_owned();

FYI, I fought a lot trying to fix these lifetime issues and eventually gaveup. From what I could tell rust looses proof that lookups don't need to outlive 'static.

---

nativelink-store/src/filesystem_store.rs line 681 at r2 (raw file):

                &PathType::Content,
                encoded_file_path.shared_context.as_ref(),
                &final_key.borrow().into_digest(),

This should take a StoreKey not a DigestInfo

---

nativelink-store/src/filesystem_store.rs line 769 at r2 (raw file):

        _upload_size: UploadSizeInfo,
    ) -> Result<(), Error> {
        let digest = key.borrow().into_digest();

ditto.

[jhpratt]

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

---

nativelink-store/src/filesystem_store.rs line 78 at r2 (raw file):

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

This should be a StoreKey<'a>

What lifetime are you envisioning? If it's 'static, then it seems possible but doesn't gain much (as anything non-trivial has to effectively be cloned). If you want to attach a lifetime to EncodedFilePath then things will get very messy very quickly.

[jhpratt]

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

a discussion (no related file):

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

The point of this PR is to enable filtering by list/range, by going into a DigestInfo we are loosing the key's range information making the call pointless.

We are also going to need to add some encoding to the name/format of the files. We probably want to prefix all files with h for digests and s for strings, then when we load them, we can turn them back to the StoreKey enum.

Note: When doing this make sure if it doesn't start with h or s to load them in as DigestInfo for reverse compatibility.

Gotcha. I changed the naming to be prefixed as suggested. Tests are currently failing, which seems to be due to the renaming. I'll be looking into this more later.

---

---

nativelink-store/src/filesystem_store.rs line 671 at r2 (raw file):

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

nit: Lets remove this comment unless there's a ticket, this will be just noise.

done

---

nativelink-store/src/filesystem_store.rs line 681 at r2 (raw file):

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

This should take a StoreKey not a DigestInfo

done

---

nativelink-store/src/filesystem_store.rs line 769 at r2 (raw file):

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

ditto.

done

[jhpratt]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), pre-commit-checks, vale, and 5 discussions need to be resolved

---

nativelink-store/tests/filesystem_store_test.rs line 821 at r3 (raw file):

    let big_digest = DigestInfo::try_new(HASH1, BIG_VALUE.len())?;

    static UNREFED_KEYS: Mutex<Vec<StoreKey<'static>>> = Mutex::new(Vec::new());

Drive-by change: Mutex::new has been const for quite a while, so there's no need to use Lazy here.

[aaronmondal]

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-init, 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, windows-2022 / stable, and 5 discussions need to be resolved

---

nativelink-store/tests/filesystem_store_test.rs line 821 at r3 (raw file):

Previously, jhpratt (Jacob Pratt) wrote…

Drive-by change: Mutex::new has been const for quite a while, so there's no need to use Lazy here.

fyi: A grep once_cell shows that the only other use of the crate is another once_cell::sync::Lazy in nativelink-worker/tests/running_actions_manager_test.rs.

If that can be removed as well we can remove the explicit once_cell dependency altogether.Might be better to defer this to another PR though.