S3 store can ignore .has() requests based on LastModified

[allada]

This feature enables users to setup s3 buckets to evict items after some N amount of time and NativeLink will ignore items that are older than some age. As long as NatliveLink's ignore time is shorter than the eviction time (plus some buffer), clients will upload the artifact again instead of assuming it exists.

closes: #35

---

This change is 

[allada]

+@caass or +@jhpratt

Reviewable status: 0 of 2 LGTMs obtained, and 0 of 8 files reviewed, 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, vale, windows-2022 / stable (waiting on @caass and @jhpratt)

[jhpratt]

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained, and all files reviewed, and pending CI: Installation / macos-13, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, and 3 discussions need to be resolved (waiting on @caass)

---

nativelink-store/Cargo.toml line 62 at r1 (raw file):

aws-smithy-runtime-api = "1.6.0"
serial_test = { version = "3.1.1", features = ["async"] }
mock_instant = "0.3.2"

Why 0.3.2? The latest version is 0.5.1.

---

nativelink-store/src/s3_store.rs line 238 at r1 (raw file):

#[derive(MetricsComponent)]
pub struct S3Store<I: InstantWrapper, NowFn: Fn() -> I + Send + Sync + Unpin + 'static> {

It's idiomatic to avoid bounds on generics on the struct itself, as it forces all implementations to repeat the bounds even when unnecessary from a technical perspective. Instead, bounds should be included only where necessary for the code to compile (barring marker traits, which isn't applicable here).

This pattern is followed throughout the standard library. For example, HashMap::new does not require that the key implements Hash.

---

nativelink-store/tests/s3_store_test.rs line 519 at r1 (raw file):

                http::Response::builder()
                    .status(StatusCode::OK)
                    .body(SdkBody::from(concat!(

Why concat! instead of writing the string directly?

[allada]

Reviewable status: 0 of 2 LGTMs obtained, and 7 of 8 files reviewed, and pending CI: Local / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, vale, and 2 discussions need to be resolved (waiting on @caass and @jhpratt)

---

nativelink-store/Cargo.toml line 62 at r1 (raw file):

Previously, jhpratt (Jacob Pratt) wrote…

Why 0.3.2? The latest version is 0.5.1.

It's copied over from other crates that use it in our code base.

We should update it, but I'd rather update them all at the same time, or else we could end up in either linking hell or bloated binaries.

---

nativelink-store/src/s3_store.rs line 238 at r1 (raw file):

Previously, jhpratt (Jacob Pratt) wrote…

It's idiomatic to avoid bounds on generics on the struct itself, as it forces all implementations to repeat the bounds even when unnecessary from a technical perspective. Instead, bounds should be included only where necessary for the code to compile (barring marker traits, which isn't applicable here).

This pattern is followed throughout the standard library. For example, HashMap::new does not require that the key implements Hash.

Done.

---

nativelink-store/tests/s3_store_test.rs line 519 at r1 (raw file):

Previously, jhpratt (Jacob Pratt) wrote…

Why concat! instead of writing the string directly?

Because the rest of this file uses concat! and some of these are much larger. I generally prefer consistency is preferred over perfection.

[jhpratt]

Reviewed 1 of 1 files at r2, all commit messages.
Dismissed @jhpratt from a discussion.
Reviewable status: 1 of 2 LGTMs obtained, and all files reviewed, 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-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), macos-13, windows-2022 / stable (waiting on @caass)

---

nativelink-store/Cargo.toml line 62 at r1 (raw file):

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

It's copied over from other crates that use it in our code base.

We should update it, but I'd rather update them all at the same time, or else we could end up in either linking hell or bloated binaries.

Makes sense. Given that it's only for testing it doesn't matter anywhere near as much as if it were part of the API.

---

nativelink-store/src/s3_store.rs line 238 at r1 (raw file):

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

Done.

It looks like the bound is still present (via the impl block) on a number of methods that don't need it. The preferred way is to either have separate impl blocks or to add where clauses on methods as necessary, with the behavior being identical.

Marking as "discussing" to avoid blocking; 'll leave the decision up to you on whether to change it now or come back to it at some point in the future.

---

nativelink-store/tests/s3_store_test.rs line 519 at r1 (raw file):

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

Because the rest of this file uses concat! and some of these are much larger. I generally prefer consistency is preferred over perfection.

Good enough for me.

[caass]

Reviewed 7 of 8 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained, and all files reviewed (waiting on @caass)

[caass]

Reviewable status: complete! 2 of 2 LGTMs obtained, and all files reviewed

[allada]

Reviewable status: complete! 2 of 2 LGTMs obtained, and all files reviewed

---

nativelink-store/src/s3_store.rs line 238 at r1 (raw file):

Previously, jhpratt (Jacob Pratt) wrote…

It looks like the bound is still present (via the impl block) on a number of methods that don't need it. The preferred way is to either have separate impl blocks or to add where clauses on methods as necessary, with the behavior being identical.

Marking as "discussing" to avoid blocking; 'll leave the decision up to you on whether to change it now or come back to it at some point in the future.

Yeah we dont follow this pattern in the rest of the code. If/when we want to fix it in the other code this will just be one-more to do.