Add redis store

[blakehatch]

Description

Added a Redis Store.

Fixes #302

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Uses the redis_test library to create a mock instance that expects certain commands to be called exactly and confirms that the data is structured correctly when being stored. Had to add custom functionality to connection configuration to make the Native Link store config system work with this mock library.

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 

[aaronmondal]

Reviewed 8 of 14 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @allada and @blakehatch)

---

cas/store/BUILD line 547 at r2 (raw file):

        "@crate_index//:tokio",
    ],
)

Newline

---

cas/store/default_store_factory.rs line 49 at r2 (raw file):

            StoreConfig::memory(config) => Arc::new(MemoryStore::new(config)),
            StoreConfig::s3_store(config) => Arc::new(S3Store::new(config)?),
            StoreConfig::redis_store(config) => {

Would be nice if this was called in a similar way as the other stores.

---

cas/store/redis_store.rs line 72 at r2 (raw file):

    async fn configure_connection(config: &config::stores::RedisStore) -> Result<Arc<Mutex<T>>, Error> {
        let conn = if let Some(use_mock) = config.use_mock {

I believe you could remove the duplicate "else" branches here making use_mock not an option but a bool.

---

cas/store/redis_store.rs line 74 at r2 (raw file):

        let conn = if let Some(use_mock) = config.use_mock {
            if use_mock {
                let mock_commands = config.mock_commands.clone().unwrap_or_default();

Since this is specific to mocking, would it make sense to move this to the impl for the MockRedisConnection?

---

cas/store/redis_store.rs line 138 at r2 (raw file):

                Ok::<(usize, _), Error>((index, exists))
            })
            .collect();

This can probably be optimized, but I don't think it matters for the initial implementation. We can always add benchmarks and optimizations later.

---

cas/store/tests/redis_store_test.rs line 59 at r2 (raw file):

    #[tokio::test]
    async fn insert_one_item_then_update_and_get() -> Result<(), Error> {
        let data1 = Bytes::from_static(b"13");

It looks like this (and the ones in the other test) can be const.

---

config/stores.rs line 454 at r2 (raw file):

    /// If store is being used in unit tests set this to true
    pub use_mock: Option<bool>,

I think this can just be a bool.

---

config/examples/basic_cas.json line 147 at r2 (raw file):

    "max_open_files": 512
  }
}

Newline/unnecessary change

[MarcusSorealheis]

I'm not sure how we could possibly merge this without documenting its usage.

[MarcusSorealheis]

NativeLink

[MarcusSorealheis]

NativeLink

[MarcusSorealheis]

NativeLink

[blakehatch]

It's usage is documented in the stores config same as all the other stores are, this info will be surfaced with rustdocs as well.

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks

---

nativelink-store/src/mock_redis.rs line 1 at r3 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

NativeLink

Good catch left that from the rebase.

---

nativelink-store/src/redis_store.rs line 1 at r3 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

NativeLink

Fixed.

---

nativelink-store/tests/redis_store_test.rs line 1 at r3 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

NativeLink

Fixed.

---

cas/store/BUILD line 547 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Newline

Done.

---

cas/store/default_store_factory.rs line 49 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Would be nice if this was called in a similar way as the other stores.

Fixed.

---

cas/store/redis_store.rs line 72 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I believe you could remove the duplicate "else" branches here making use_mock not an option but a bool.

Removed all Mock integrations to an extremely minimal external file, not extremely happy with this and would like to move these trait implementations to the test crate, however you can't impl outside of the crate itself. If there are any patterns that anyone recommends to clean up this would be amazing.

---

cas/store/redis_store.rs line 74 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Since this is specific to mocking, would it make sense to move this to the impl for the MockRedisConnection?

Was actually able to extract this section to a function that just lives in the redis_test file.

---

cas/store/redis_store.rs line 138 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

This can probably be optimized, but I don't think it matters for the initial implementation. We can always add benchmarks and optimizations later.

Yeah there are a few optimizations, I also upgraded the crate and used a better connection type that they recommend called MultiplexedConnection, I imagine a lot of the optimizations would be around utilizing this effectively. Would love benchmarks on this though especially as we test different Redis API compatible stores.

---

cas/store/tests/redis_store_test.rs line 59 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

It looks like this (and the ones in the other test) can be const.

Rebase changed file locations not sure what line this is referring to.

---

config/stores.rs line 454 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I think this can just be a bool.

Ditto

---

config/examples/basic_cas.json line 147 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Newline/unnecessary change

Fixed.

[adam-singer]

Reviewed 3 of 27 files at r3, 2 of 5 files at r4.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks

---

nativelink-config/src/stores.rs line 599 at r4 (raw file):

    /// The hostname or IP address of the Redis server can include username, passowrd, tls, and database number.
    #[serde(default, deserialize_with = "convert_string_with_shellexpand")]
    pub url: String,

to be consistent with other parts of the config I think this should be called "address", GrpcEndpoint and HttpListener use that term. If there are other fields such as tls should that be different structures?

[blakehatch]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Vercel, pre-commit-checks

---

nativelink-config/src/stores.rs line 599 at r4 (raw file):

Previously, adam-singer (Adam Singer) wrote…

to be consistent with other parts of the config I think this should be called "address", GrpcEndpoint and HttpListener use that term. If there are other fields such as tls should that be different structures?

Done. I can add options to enable and disable TLS, backoff, retry, etc. Lots of options in the crate idk if expanding this is worth just creating another PR for.

[allada]

Reviewed 6 of 27 files at r3, 1 of 5 files at r4, 2 of 3 files at r6, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks

---

nativelink-config/src/stores.rs line 599 at r4 (raw file):

Previously, blakehatch (Blake Hatch) wrote…

Done. I can add options to enable and disable TLS, backoff, retry, etc. Lots of options in the crate idk if expanding this is worth just creating another PR for.

Lets do it as a different PR, as it'll make reviews easier.

---

nativelink-config/src/stores.rs line 166 at r6 (raw file):

    grpc(GrpcStore),

    /// Stores data in a Redis store or any stores compatible with Redis APIs.

nit: No point in saying "Redis store or any store compatible..." just say "any store compatible ...".

---

nativelink-config/src/stores.rs line 170 at r6 (raw file):

    /// This store pairs well with the SizePartitioning store to accept only
    /// small data that is optimally sized to fit in the Redis store. As Redis
    /// and other in-memory stores are generally fast as well it is also

The way this reads is strange. Instead just say it pairs well with SizePartitioning and/or FastSlow then say reasons and other limitations.

One limitation that is worth noting here is that most redis implementations have size limits per object, which is an easy foot-gun for people that don't know that.

---

nativelink-config/src/stores.rs line 597 at r6 (raw file):

#[derive(Serialize, Deserialize, Debug, Clone)]
pub struct RedisStore {
    /// The hostname or IP address of the Redis server can include username, passowrd, tls, and database number.

nit: Please give an example.

---

nativelink-config/src/stores.rs line 599 at r6 (raw file):

    /// The hostname or IP address of the Redis server can include username, passowrd, tls, and database number.
    #[serde(default, deserialize_with = "convert_string_with_shellexpand")]
    pub address: String,

even though the redis client implementation for rust does not support multiple multiplexing on different addresses, I think we should future-proof our config by making this a Vec<String> and documenting that it currently only exactly 1 address and in the code enforce it. This will prevent a future breaking change.

In the future adding manual multiplexing should be straight forward.

---

nativelink-error/BUILD.bazel line 11 at r6 (raw file):

    deps = [
        "//nativelink-proto",
        #"@crates//:futures-rustls",

Remove.

---

nativelink-error/BUILD.bazel line 15 at r6 (raw file):

        "@crates//:prost",
        "@crates//:prost-types",
        "@crates//:redis",

Sadly, we cannot/shouldnot do this. We'll have to write a helper for this.

It doesn't make sense to force every create to import redis just to convert the error types for a single store implementation detail.

---

nativelink-error/src/lib.rs line 123 at r6 (raw file):

impl From<redis::RedisError> for Error {
    fn from(err: redis::RedisError) -> Self {
        Error::new(Code::Internal, format!("Redis error: {}", err))

fyi: I'm a bit afraid that we might mask a lot of these in Internal errors when they are not-founds or other kind of errors that are not internal.

---

nativelink-store/BUILD.bazel line 53 at r6 (raw file):

        "@crates//:filetime",
        "@crates//:futures",
        #"@crates//:futures-rustls",

nit: Remove.

---

nativelink-store/BUILD.bazel line 112 at r6 (raw file):

        "@crates//:filetime",
        "@crates//:futures",
        #"@crates//:futures-rustls",

nit: Remove.

---

nativelink-store/Cargo.toml line 31 at r6 (raw file):

rand = "0.8.5"
redis = { version = "0.25.2", features = ["tokio-comp", "tokio-rustls-comp"] }
redis-test = { version = "0.4.0", features = ["aio"] }

I think this should be a dev-dependency or at least masked behind a cfg test.

---

nativelink-store/src/default_store_factory.rs line 25 at r6 (raw file):

use nativelink_util::metrics_utils::Registry;
use nativelink_util::store_trait::Store;
use redis::aio::MultiplexedConnection;

This should be under RedisStore, no code outside of redis_store.rs or redis_store_test.rs should import the redis create.

---

nativelink-store/src/mock_redis.rs line 1 at r6 (raw file):

// Copyright 2024 The NativeLink Authors. All rights reserved.

This file is small enough to just put it in with the test.

---

nativelink-store/src/redis_store.rs line 37 at r6 (raw file):

    type ConnType: Send + Sync + Sized + 'static;

    async fn new_with_config(config: &nativelink_config::stores::RedisStore)

nit: These are public APIs so they should be documented.

---

nativelink-store/src/redis_store.rs line 40 at r6 (raw file):

        -> Result<Self, Error>;

    async fn new_with_connection(conn: Self::ConnType) -> Result<Self, Error>;

nit: ditto.

---

nativelink-store/src/redis_store.rs line 51 at r6 (raw file):

    ) -> Result<Self, Error> {
        let client = redis::Client::open(config.url.clone());
        let connection = client?.get_multiplexed_tokio_connection().await?;

Shouldn't we use get_multiplexed_tokio_connection_with_response_timeouts? Otherwise won't

---

nativelink-store/src/redis_store.rs line 51 at r6 (raw file):

    ) -> Result<Self, Error> {
        let client = redis::Client::open(config.url.clone());
        let connection = client?.get_multiplexed_tokio_connection().await?;

nit: Use .err_tip() to give searchable points in code instead of straight question-marks.

---

nativelink-store/src/redis_store.rs line 58 at r6 (raw file):

        Err(Error::new(
            Code::Unimplemented,
            "new_with_connection method is not implemented".to_string(),

This is weird, then why create the function in the first place....

I believe you are looking for the Builder pattern to give you what you are looking for.

---

nativelink-store/src/redis_store.rs line 63 at r6 (raw file):

}

pub struct RedisStore<T: RedisConnectionTrait + 'static> {

nit: I don't think you need all these 'static, since RedisConnectionTrait already requires 'static.

---

nativelink-store/src/redis_store.rs line 78 at r6 (raw file):

    pub async fn new_with_connection(conn: T) -> Result<Self, Error> {
        Ok(RedisStore {
            conn: Arc::new(Mutex::new(conn)),

An Arc<Mutex<Conn>>> does not feel like the right approach here. Connection is fairly light weight to be cloned. I believe what would work better is to either always clone the Connection (easiest, but likely less efficient), or to use a Semaphore with a permit system to recycle Connections.

The way the code is written below would only allow one async request to be outsanding at a time pretty removing the power of redis. What should be happening is have the code below grab a unique connection, use it, then either discard it or return it to the pool.

---

nativelink-store/src/redis_store.rs line 126 at r6 (raw file):

        };

        let buffer = reader

nit: This will likely cause lots of memory issues.

What we probably want to do instead is use transactions and APPEND.

IOW, create a transaction, then read the reader stream and keep calling APPEND on the same key, then commit or discard the transaction.

---

nativelink-store/src/redis_store.rs line 148 at r6 (raw file):

        let mut conn = conn.lock().await;
        let packed_hash = encode(digest.packed_hash);
        let value = conn.get::<_, Vec<u8>>(&packed_hash).await?;

fyi: Example, if we are downloading 50 megs of data here, no-one else can download, upload or check the existence of any other items in the redis store because conn is a MutexGuard and currently locked.

[adam-singer]

Reviewed 4 of 12 files at r7, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), pre-commit-checks, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 29 discussions need to be resolved

[allada]

Reviewed 2 of 7 files at r9, 1 of 7 files at r11, 4 of 6 files at r12, 7 of 7 files at r13, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 22 discussions need to be resolved

---

nativelink-config/src/stores.rs line 601 at r12 (raw file):

#[derive(Serialize, Deserialize, Debug, Clone)]
pub struct RedisStore {
    //(TODO: Reduce to 80 characters)

reminder

---

nativelink-config/src/stores.rs line 608 at r12 (raw file):

    pub addresses: Vec<String>,

    #[serde(default)]

nit: Styling, we put these under the comments.

---

nativelink-store/src/redis_store.rs line 75 at r13 (raw file):

        // Start connecting to redis, but don't block our construction on it.
        tokio::spawn(conn_fut.clone());

nit: Lets wrap this and print any errors this future returns to error!(), this will give a nice startup error if it cannot connect, rather than having to wait for a request to come in before erroring.

---

nativelink-store/src/redis_store.rs line 136 at r13 (raw file):

            .query_async::<_, Vec<usize>>(&mut conn)
            .await
            .map_err(from_redis_err)?;

nit: Needs err_tip

---

nativelink-store/src/redis_store.rs line 170 at r13 (raw file):

        let mut first_run = true;
        while first_run || !reader.is_empty() {

There's a bug here, this needs to be wrapped in another loop.

Sadly because this was caught in code review we need to write a test for this too.

The condition case is we need to send a few packets in, but then yield, then send the rest of the packets. With the way the code is written right now, it won't receive those last packets and it'll commit them partially.

---

nativelink-store/src/redis_store.rs line 198 at r13 (raw file):

            tokio::task::yield_now().await;

            pipe.query_async(&mut conn)

once the above comment is addressed, this needs to be moved to the outer loop.

---

nativelink-store/tests/redis_store_test.rs line 60 at r13 (raw file):

                    command.arg(arg);
                }
                for res in result.as_ref().unwrap().iter() {

nit: Why .unwrap() here? Maybe using RedisResult is not the right one?

---

nativelink-store/tests/redis_store_test.rs line 62 at r13 (raw file):

                for res in result.as_ref().unwrap().iter() {
                    res_vec.push(res.clone());
                    println!("{:?}", res.clone());

nit: Remove.

---

nativelink-store/tests/redis_store_test.rs line 135 at r13 (raw file):

        let data = Bytes::from_static(b"");

        let digest = DigestInfo::new(

make ZERO_BYTE_DIGESTS public then import it here, so we don't need to do this.

---

nativelink-store/tests/redis_store_test.rs line 143 at r13 (raw file):

            0,
        );
        let dl = data.len();

nit: unused.

---

nativelink-store/tests/redis_store_test.rs line 144 at r13 (raw file):

        );
        let dl = data.len();
        println!("DATA LEN: {dl:?}");

nit: Remove.

---

nativelink-store/tests/redis_store_test.rs line 148 at r13 (raw file):

        let redis_connection = MockRedisConnectionBuilder::new()
            .cmd("RENAME", &[TEMP_UUID, &digest.clone().hash_str()], Ok("1"))
            .pipe(&[(

this one should not happen. I'm pretty sure it should not have any redis calls.

[blakehatch]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 16 discussions need to be resolved

---

nativelink-config/src/stores.rs line 601 at r12 (raw file):

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

reminder

Done.

---

nativelink-config/src/stores.rs line 608 at r12 (raw file):

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

nit: Styling, we put these under the comments.

Done.

---

nativelink-store/src/redis_store.rs line 75 at r13 (raw file):

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

nit: Lets wrap this and print any errors this future returns to error!(), this will give a nice startup error if it cannot connect, rather than having to wait for a request to come in before erroring.

Done.

---

nativelink-store/src/redis_store.rs line 136 at r13 (raw file):

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

nit: Needs err_tip

Done.

---

nativelink-store/src/redis_store.rs line 170 at r13 (raw file):

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

There's a bug here, this needs to be wrapped in another loop.

Sadly because this was caught in code review we need to write a test for this too.

The condition case is we need to send a few packets in, but then yield, then send the rest of the packets. With the way the code is written right now, it won't receive those last packets and it'll commit them partially.

Done.

---

nativelink-store/src/redis_store.rs line 198 at r13 (raw file):

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

once the above comment is addressed, this needs to be moved to the outer loop.

Done.

---

nativelink-store/tests/redis_store_test.rs line 60 at r13 (raw file):

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

nit: Why .unwrap() here? Maybe using RedisResult is not the right one?

The pipe returns a RedisResult which contains multiple items when the value is a Bulk, I then need to pass these into MockCmd::with_values which takes a vec of results so I need to unwrap to convert the value into the vec form.

    fn execute_transaction(&self, con: &mut dyn ConnectionLike) -> RedisResult<Value> {
        let mut resp = con.req_packed_commands(
            &encode_pipeline(&self.commands, true),
            self.commands.len() + 1,
            1,
        )?;
        match resp.pop() {
            Some(Value::Nil) => Ok(Value::Nil),
            Some(Value::Bulk(items)) => Ok(self.make_pipeline_results(items)),
            _ => fail!((
                ErrorKind::ResponseError,
                "Invalid response when parsing multi response"
            )),
        }

---

nativelink-store/tests/redis_store_test.rs line 62 at r13 (raw file):

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

nit: Remove.

Done.

---

nativelink-store/tests/redis_store_test.rs line 135 at r13 (raw file):

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

make ZERO_BYTE_DIGESTS public then import it here, so we don't need to do this.

Done.

---

nativelink-store/tests/redis_store_test.rs line 143 at r13 (raw file):

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

nit: unused.

Done.

---

nativelink-store/tests/redis_store_test.rs line 144 at r13 (raw file):

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

nit: Remove.

Done.

---

nativelink-store/tests/redis_store_test.rs line 148 at r13 (raw file):

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

this one should not happen. I'm pretty sure it should not have any redis calls.

Done. It actually wasn't calling them I didn't realize the test crate doesn't error out from extra calls just when it doesn't match up the calls that happen.

[allada]

Reviewed 4 of 4 files at r14, all commit messages.
Dismissed @MarcusSorealheis and @aaronmondal from 3 discussions.
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, 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, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 12 discussions need to be resolved

---

nativelink-store/tests/redis_store_test.rs line 232 at r14 (raw file):

    #[tokio::test]
    async fn yield_between_sending_packets_in_update() -> Result<(), Error> {
        let data: Bytes = Bytes::from(vec![0u8; 10 * 1024]);

nit: Don't declare the type when it's already clear.

[allada]

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, 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, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 12 discussions need to be resolved

[allada]

Dismissed @MarcusSorealheis from a discussion.
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, 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, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 11 discussions need to be resolved

[allada]

Dismissed @aaronmondal from 6 discussions.
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, 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, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 10 discussions need to be resolved

[blakehatch]

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, 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, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 9 discussions need to be resolved

---

nativelink-store/tests/redis_store_test.rs line 232 at r14 (raw file):

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

nit: Don't declare the type when it's already clear.

Done.

Approved

[adam-singer]

Reviewed 6 of 6 files at r15, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 9 discussions need to be resolved

[allada]

Reviewed all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 9 discussions need to be resolved

[allada]

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), zig-cc ubuntu-20.04, zig-cc ubuntu-22.04, and 10 discussions need to be resolved

---

nativelink-store/tests/redis_store_test.rs line 34 at r15 (raw file):

fn mock_uuid_generator() -> String {
    uuid::Uuid::parse_str("550e8400-e29b-41d4-a716-446655440000")

nit: try something like:

const UUID: &str = "550e8400-e29b-41d4-a716-446655440000";
const TEMP_UUID: &str = concat!("temp-", UUID);

fn mock_uuid_generator() -> String {
  UUID.to_string()
}

Or better yet replace all mock_uuid_generator calls with || UUID.to_string()

[aaronmondal]

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