Add optional pub sub publisher to store writes

[blakehatch]

Description

This optional publisher for stores writes is a cleaner, more flexible, and simpler alternative to using keyspace notifications.

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

---

This change is 

[caass]

Awesome :) I love it !! I had like one blocking concern and mostly a few minor nits.

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

---

-- commits line 2 at r1:
Minor: Can you make this more descriptive? Like at least mention that it's for the Redis store.

---

nativelink-config/src/stores.rs line 881 at r1 (raw file):

    /// Default: 10
    #[serde(default)]
    pub connection_timeout_s: u64,

FYI unrelated to this current PR, these should be like. Durations deserialized with serde_with or something. Whatever.

---

nativelink-config/src/stores.rs line 883 at r1 (raw file):

    pub connection_timeout_s: u64,

    /// An optional channel to publish writes to redis.

Nit: Can you clarify this a little bit? Maybe like

/// An optional Redis channel to publish write events to.
///
/// If set, every time a write operation is made to a Redis node then an event will be published to a Redis channel with the given name.
/// If unset, the writes will still be made, but the write events will not be published.
///
/// Default: (Empty String / No Channel)

---

nativelink-store/src/redis_store.rs line 97 at r1 (raw file):

impl<T: ConnectionLike + Unpin + Clone + Send + Sync> RedisStore<T> {
    pub fn new_with_conn_and_name_generator(

Minor: Can you rename this (and i think there's another method we should actually delete probably? Like we should probably have one method that's like new(config: &Config) and one that's with_conn_and_pub_sub_channel_and_temp_name_generator_fn

---

nativelink-store/src/redis_store.rs line 266 at r1 (raw file):

                .await
                .map_err(from_redis_err)
                .err_tip(|| "Failed to publish temp key value to configured channel")?;

Try:

if let Some(ref channel) = self.pub_sub_channel {
// more code...
}

Also: for the call to publish, you shouldn't use the Debug representation -- use the Display representation instead. Check this out: https://doc.rust-lang.org/rust-by-example/hello/print.html.

Also also: it seems bizarre to me that the way to publish here is to basically upload the key, upload the value, and then also in a separate transaction upload the value again. Is that really what we have to do? Can't we just like. Upload the value once? I don't know the answer to this it just seems like we should be able to execute those all at once. Maybe there's a write-and-publish operation?

Code quote:

        if !self.pub_sub_channel.is_none() {
            let temp_key_value = conn.get(temp_key.get_or_init(make_temp_name)).await?;

            conn.publish(&self.pub_sub_channel, format!("{:?} {:?}", key, temp_key_value))
                .await
                .map_err(from_redis_err)
                .err_tip(|| "Failed to publish temp key value to configured channel")?;

[blakehatch]

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

---

-- commits line 2 at r1:

Previously, caass (Cass Fridkin) wrote…

Minor: Can you make this more descriptive? Like at least mention that it's for the Redis store.

Oh yes that was a mistype this is not a store trait feature haha

---

nativelink-store/src/redis_store.rs line 97 at r1 (raw file):

Previously, caass (Cass Fridkin) wrote…

Minor: Can you rename this (and i think there's another method we should actually delete probably? Like we should probably have one method that's like new(config: &Config) and one that's with_conn_and_pub_sub_channel_and_temp_name_generator_fn

This is a weird holdover from how the mocking has to be done to make the UUID generation deterministic you have to pass in a name_generator. I'm not opposed to changing it to be cleaner if possible but it would probably be best suited for a separate PR.

---

nativelink-store/src/redis_store.rs line 266 at r1 (raw file):

Previously, caass (Cass Fridkin) wrote…

Try:

if let Some(ref channel) = self.pub_sub_channel {
// more code...
}

Also: for the call to publish, you shouldn't use the Debug representation -- use the Display representation instead. Check this out: https://doc.rust-lang.org/rust-by-example/hello/print.html.

Also also: it seems bizarre to me that the way to publish here is to basically upload the key, upload the value, and then also in a separate transaction upload the value again. Is that really what we have to do? Can't we just like. Upload the value once? I don't know the answer to this it just seems like we should be able to execute those all at once. Maybe there's a write-and-publish operation?

The issue with this is that we don't call SET to put all the data up we incrementally build up the data with APPEND and then at the end swap out the temporary key. So essentially I have to wait for it to be built up and then when the key swap happens I then grab the full value to publish. Shouldn't be a huge deal performance-wise since this is an optional feature but I agree it would be nice to do both at the same time.

[adam-singer]

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

---

nativelink-config/src/stores.rs line 881 at r1 (raw file):

Previously, caass (Cass Fridkin) wrote…

FYI unrelated to this current PR, these should be like. Durations deserialized with serde_with or something. Whatever.

Good point, lets move this discussion out of this PR. There have been prior discussions around friendly formats for number formats that has landed. We can bring up Duration for replacing integers offline, I assume we would also get a friendly parsing scheme when making that move.

[caass]

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

---

-- commits line 2 at r1:

Previously, blakehatch (Blake Hatch) wrote…

Oh yes that was a mistype this is not a store trait feature haha

:)

---

nativelink-config/src/stores.rs line 881 at r1 (raw file):

Previously, adam-singer (Adam Singer) wrote…

Good point, lets move this discussion out of this PR. There have been prior discussions around friendly formats for number formats that has landed. We can bring up Duration for replacing integers offline, I assume we would also get a friendly parsing scheme when making that move.

Totally, I just saw it on my reviewable and I was like "aaaghhh!!" but yes not a blocker here :)

---

nativelink-store/src/redis_store.rs line 97 at r1 (raw file):

Previously, blakehatch (Blake Hatch) wrote…

This is a weird holdover from how the mocking has to be done to make the UUID generation deterministic you have to pass in a name_generator. I'm not opposed to changing it to be cleaner if possible but it would probably be best suited for a separate PR.

Sure

---

nativelink-store/src/redis_store.rs line 266 at r1 (raw file):

Previously, blakehatch (Blake Hatch) wrote…

The issue with this is that we don't call SET to put all the data up we incrementally build up the data with APPEND and then at the end swap out the temporary key. So essentially I have to wait for it to be built up and then when the key swap happens I then grab the full value to publish. Shouldn't be a huge deal performance-wise since this is an optional feature but I agree it would be nice to do both at the same time.

I think you can probably make it part of the same pipeline? During my work to implement cluster mode, I did some research around like. How transactions and stuff works in redis -- pipelining allows you to submit multiple commands over the same TCP request, which would make me feel a lot better. That's where we have redis::pipe() and then add commands to the Pipe, before finally sending it over the connection.

Also, one time when I tried to do this I got some borrow checker problems because of moving the pipe in and out of async blocks -- so let me know if that's the case.

[zbirenbaum]

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

---

nativelink-store/src/redis_store.rs line 266 at r1 (raw file):

Previously, caass (Cass Fridkin) wrote…

I think you can probably make it part of the same pipeline? During my work to implement cluster mode, I did some research around like. How transactions and stuff works in redis -- pipelining allows you to submit multiple commands over the same TCP request, which would make me feel a lot better. That's where we have redis::pipe() and then add commands to the Pipe, before finally sending it over the connection.

Also, one time when I tried to do this I got some borrow checker problems because of moving the pipe in and out of async blocks -- so let me know if that's the case.

@caass Just to make sure I'm understanding this correctly, are the two uploads you are referring to the steps where:

1. value is appended to temp-key
2. value in temp-key is assigned to actual key through rename

The sync transaction blocks where you can get a value and then perform some operation on it might be a good fit here, but pipelining alone won't allow you to do a get + set gotten value, and I think redis-rs has some issues with transaction blocks + async...

@blakehatch If you have to wait have to wait for the key swap to publish, why not do publication after the rename? Right now this code looks very much like it's publishing the temp value:

            conn.publish(&self.pub_sub_channel, format!("{:?} {:?}", key, temp_key_value))
                .await
                .map_err(from_redis_err)
                .err_tip(|| "Failed to publish temp key value to configured channel")?;

It might be worthwhile to put some more documentation for the logic here and maybe do the publish step after rename so that it's clearer we are publishing the final value

[caass]

Nice work rebasing, sorry I made about a billion changes here. @blakehatch let's maybe pair to remove the hangup I have over pipelining the PUBLISH command?

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

---

nativelink-config/src/stores.rs line 883 at r1 (raw file):

Previously, caass (Cass Fridkin) wrote…

Nit: Can you clarify this a little bit? Maybe like

/// An optional Redis channel to publish write events to.
///
/// If set, every time a write operation is made to a Redis node then an event will be published to a Redis channel with the given name.
/// If unset, the writes will still be made, but the write events will not be published.
///
/// Default: (Empty String / No Channel)

LGTM

---

nativelink-service/tests/bep_server_test.rs line 97 at r3 (raw file):

    };
    let store_key = StoreKey::Str(Cow::Owned(format!(
        "{}-{}-{}-L",

What's up with the -L here and -B elsewhere?

---

nativelink-store/src/redis_store.rs line 266 at r1 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

@caass Just to make sure I'm understanding this correctly, are the two uploads you are referring to the steps where:

1. value is appended to temp-key
2. value in temp-key is assigned to actual key through rename

The sync transaction blocks where you can get a value and then perform some operation on it might be a good fit here, but pipelining alone won't allow you to do a get + set gotten value, and I think redis-rs has some issues with transaction blocks + async...

@blakehatch If you have to wait have to wait for the key swap to publish, why not do publication after the rename? Right now this code looks very much like it's publishing the temp value:

            conn.publish(&self.pub_sub_channel, format!("{:?} {:?}", key, temp_key_value))
                .await
                .map_err(from_redis_err)
                .err_tip(|| "Failed to publish temp key value to configured channel")?;

It might be worthwhile to put some more documentation for the logic here and maybe do the publish step after rename so that it's clearer we are publishing the final value

@zbirenbaum No,I'm talking about how we call conn.publish() instead of pipe.cmd("PUBLISH"). My gut feeling is that redis doesn't mind making the PUBLISH command part of the same transaction? @blakehatch did that come up at all for you?

[allada]

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

---

nativelink-config/examples/basic_cas.json line 37 at r3 (raw file):

      }
    },
    "BEP_STORE": {

nit: Don't put this as a default. This is the baseline of running NL without any dependencies.

---

nativelink-config/src/stores.rs line 891 at r3 (raw file):

    /// Default: (Empty String / No Channel)
    #[serde(default)]
    pub pub_sub_channel: Option<String>,

nit: mark this experimental_

---

nativelink-service/src/bep_server.rs line 76 at r3 (raw file):

            .update_oneshot(
                StoreKey::Str(Cow::Owned(format!(
                    "{}-{}-{}-L",

What is L?

---

nativelink-service/src/bep_server.rs line 112 at r3 (raw file):

                .err_tip(|| "Could not encode PublishBuildToolEventStreamRequest proto")?;

            if !buf.is_empty() {

nit: Shouldn't we publish empty protos?

---

nativelink-service/src/bep_server.rs line 116 at r3 (raw file):

                    .update_oneshot(
                        StoreKey::Str(Cow::Owned(format!(
                            "{}-{}-{}-B",

what is B?

---

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

                if self.pub_sub_channel.is_some() {
                    conn.publish(&self.pub_sub_channel, format!("{:?} {:?}", key, &chunk[..]))

nit: You probably don't want to publish a Some<String>. You probably should unwrap it in the if-statement.

---

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

                if self.pub_sub_channel.is_some() {
                    conn.publish(&self.pub_sub_channel, format!("{:?} {:?}", key, &chunk[..]))

I don't think we should publish the data. We should publish the key and let the receiver lookup the data when it's received.

Yes this does mean they might get an old version, but we currently don't need to support receiving every event. We only currently care about most recent event.

[blakehatch]

Thanks! Appreciate all the thoughts :)

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, 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 8 discussions need to be resolved

---

nativelink-config/src/stores.rs line 883 at r1 (raw file):

Previously, caass (Cass Fridkin) wrote…

Nit: Can you clarify this a little bit? Maybe like

/// An optional Redis channel to publish write events to.
///
/// If set, every time a write operation is made to a Redis node then an event will be published to a Redis channel with the given name.
/// If unset, the writes will still be made, but the write events will not be published.
///
/// Default: (Empty String / No Channel)

Clarified, also added that it is experimental since it is mostly a nice to have for BEP.

---

nativelink-store/src/redis_store.rs line 266 at r1 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

@caass Just to make sure I'm understanding this correctly, are the two uploads you are referring to the steps where:

1. value is appended to temp-key
2. value in temp-key is assigned to actual key through rename

The sync transaction blocks where you can get a value and then perform some operation on it might be a good fit here, but pipelining alone won't allow you to do a get + set gotten value, and I think redis-rs has some issues with transaction blocks + async...

@blakehatch If you have to wait have to wait for the key swap to publish, why not do publication after the rename? Right now this code looks very much like it's publishing the temp value:

            conn.publish(&self.pub_sub_channel, format!("{:?} {:?}", key, temp_key_value))
                .await
                .map_err(from_redis_err)
                .err_tip(|| "Failed to publish temp key value to configured channel")?;

It might be worthwhile to put some more documentation for the logic here and maybe do the publish step after rename so that it's clearer we are publishing the final value

I've been testing it to just dump the bytes currently in the chunk over to the receiving end and I haven't had any issues. I've been having some issues with grabbing the value at the time of the swap and is a performance hit as it requires an entire additional redis operation instead of leveraging the data in the chunk being passed around.

As this is experimental and for BEP which benefits from performance over mild stability increases here (occasionally maybe getting a partial message). I'd rather use this current setup esp as I've never seen a BEP Event message not fit in the chunk.

---

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

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

I don't think we should publish the data. We should publish the key and let the receiver lookup the data when it's received.

Yes this does mean they might get an old version, but we currently don't need to support receiving every event. We only currently care about most recent event.

I actually ended up needing every event hence why I made this feature for myself.

[blakehatch]

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, 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 8 discussions need to be resolved

---

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

Previously, blakehatch (Blake Hatch) wrote…

I actually ended up needing every event hence why I made this feature for myself.

I can maybe switch it to this later but this is far simpler and again is optional. Would make getting V1 of BEP over the line far easier

[blakehatch]

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, 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 8 discussions need to be resolved

---

nativelink-store/src/redis_store.rs line 266 at r1 (raw file):

Previously, blakehatch (Blake Hatch) wrote…

I've been testing it to just dump the bytes currently in the chunk over to the receiving end and I haven't had any issues. I've been having some issues with grabbing the value at the time of the swap and is a performance hit as it requires an entire additional redis operation instead of leveraging the data in the chunk being passed around.

As this is experimental and for BEP which benefits from performance over mild stability increases here (occasionally maybe getting a partial message). I'd rather use this current setup esp as I've never seen a BEP Event message not fit in the chunk.

Swapped to pipe, it functions equivalently

[caass]

Ofc :)

Reviewed 1 of 1 files at r4, all commit messages.
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, and 7 discussions need to be resolved

---

nativelink-store/src/redis_store.rs line 266 at r1 (raw file):

Previously, blakehatch (Blake Hatch) wrote…

Swapped to pipe, it functions equivalently

swag

[blakehatch]

🤗

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

---

nativelink-config/examples/basic_cas.json line 37 at r3 (raw file):

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

nit: Don't put this as a default. This is the baseline of running NL without any dependencies.

Removed all from basic config

---

nativelink-config/src/stores.rs line 891 at r3 (raw file):

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

nit: mark this experimental_

Done.

---

nativelink-service/src/bep_server.rs line 76 at r3 (raw file):

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

What is L?

Indicates that the protobuf being stored in a LifeCycle object and not a BuildEvent object. They were also overwriting each other so redis was never saving the last build object it was being overwritten by the less granular and useful lifecycle object.

Also indicates how to deserialize them instead of having to use try catch style fallbacks for deserializing the two options.

---

nativelink-service/src/bep_server.rs line 112 at r3 (raw file):

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

nit: Shouldn't we publish empty protos?

I don't really see why it would be useful but I can understand not putting any filtering at this stage. Tbh tho anything ingesting it only cares about a small subset of these which definitely does not include empty protos. If there's some debugging reason this would be helpful tho I can understand that.

---

nativelink-service/src/bep_server.rs line 116 at r3 (raw file):

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

what is B?

It is indicating that the protobuf in redis is a PublishBuildToolEventStreamRequest, also prevents the final one from being overwritten.

---

nativelink-service/tests/bep_server_test.rs line 97 at r3 (raw file):

Previously, caass (Cass Fridkin) wrote…

What's up with the -L here and -B elsewhere?

It is indicating that the protobuf in redis is a PublishBuildToolEventStreamRequest (-B) or PublishLifecycleEventRequest (-L), also prevents the final one from being overwritten.

---

nativelink-store/src/redis_store.rs line 266 at r1 (raw file):

Previously, caass (Cass Fridkin) wrote…

swag

😎

---

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

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

nit: You probably don't want to publish a Some<String>. You probably should unwrap it in the if-statement.

It's not a Some, it's an Option and is guaranteed to be something because of the conditional it's in.

[allada]

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

---

nativelink-service/src/bep_server.rs line 76 at r3 (raw file):

Previously, blakehatch (Blake Hatch) wrote…

Indicates that the protobuf being stored in a LifeCycle object and not a BuildEvent object. They were also overwriting each other so redis was never saving the last build object it was being overwritten by the less granular and useful lifecycle object.

Also indicates how to deserialize them instead of having to use try catch style fallbacks for deserializing the two options.

I see, in that case we should make a function that names this, so the code becomes self-explanatory.

---

nativelink-service/src/bep_server.rs line 112 at r3 (raw file):

Previously, blakehatch (Blake Hatch) wrote…

I don't really see why it would be useful but I can understand not putting any filtering at this stage. Tbh tho anything ingesting it only cares about a small subset of these which definitely does not include empty protos. If there's some debugging reason this would be helpful tho I can understand that.

I would not put this branch in.

protobufs are weird, an empty proto actually can have data. For example a proto of:

{
  id: 0,
  found_items: [],
  prefix: "",
}

will be an empty proto, but this proto might have meaning.

---

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

Previously, blakehatch (Blake Hatch) wrote…

It's not a Some, it's an Option and is guaranteed to be something because of the conditional it's in.

I'm saying:

if let Some(pub_sub_channel) = &self.pub_sub_channel {
  pipe.publish(pub_sub_channel, format!("{:?} {:?}", key, &chunk[..]));
}

Otherwise you are publishing the optional and not the actual value.

---

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

Previously, blakehatch (Blake Hatch) wrote…

I can maybe switch it to this later but this is far simpler and again is optional. Would make getting V1 of BEP over the line far easier

In that case, we should make a new key for every item.

example: {build_id}-{event_id}-{index}

[blakehatch]

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

---

nativelink-service/src/bep_server.rs line 76 at r3 (raw file):

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

I see, in that case we should make a function that names this, so the code becomes self-explanatory.

I can write -Lifecycle or something instead of -L. It's already contained in a function that clearly indicates which of the two is being sent it's just unclear that -L is indicating that.

---

nativelink-service/src/bep_server.rs line 112 at r3 (raw file):

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

I would not put this branch in.

protobufs are weird, an empty proto actually can have data. For example a proto of:

{
  id: 0,
  found_items: [],
  prefix: "",
}

will be an empty proto, but this proto might have meaning.

Makes sense and shouldn't cause too much strain I haven't seen any empty protobufs get sent in my testing anyways.

---

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

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

In that case, we should make a new key for every item.

example: {build_id}-{event_id}-{index}

Ok I'll switch to this cuz it would be nice to be able to reconstruct the full history if the Microservice ever goes down and we just get forget with no fire.

---

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

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

I'm saying:

if let Some(pub_sub_channel) = &self.pub_sub_channel {
  pipe.publish(pub_sub_channel, format!("{:?} {:?}", key, &chunk[..]));
}

Otherwise you are publishing the optional and not the actual value.

Makes sense and still working fine same as before I didn't accidentally adjust to the option on the other end haha. Done.

[allada]

Reviewed 2 of 3 files at r7.
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, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, 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-service/src/bep_server.rs line 60 at r7 (raw file):

        invocation_id: &'a str,
        index: i64,
    ) -> StoreKey<'a> {

nit: The contract is actually:

fn create_store_key(
        event_type: &str,
        build_id: &str,
        invocation_id: &str,
        index: i64,
    ) -> StoreKey<'static>

---

nativelink-service/src/bep_server.rs line 61 at r7 (raw file):

        index: i64,
    ) -> StoreKey<'a> {
        StoreKey::Str(Cow::Owned(format!(

nit: lol, now I would inline this, since you are not suffixing it with -L and it's self-explanatory now if inlined.

---

nativelink-service/tests/bep_server_test.rs line 199 at r7 (raw file):

                ordered_build_event: Some(OrderedBuildEvent {
                    stream_id: Some(stream_id.clone()),
                    sequence_number: 1,

This looks wrong. Why is it not passing the sequence number any more?

---

nativelink-service/tests/bep_server_test.rs line 298 at r7 (raw file):

            // First, check if the response matches what we expect.
            assert_eq!(response.sequence_number, sequence_number);

nit: Why is this changed?

---

nativelink-store/src/redis_store.rs line 500 at r7 (raw file):

        if let Some(pub_sub_channel) = &self.pub_sub_channel {
            conn.publish(pub_sub_channel, format!("{final_key}"))

nit: try final_key.into() or &final_key (or see if it'll take it without conversion.

[blakehatch]

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Installation / ubuntu-22.04, Publish image, Vercel, asan / ubuntu-22.04, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, vale, and 2 discussions need to be resolved

---

nativelink-service/src/bep_server.rs line 60 at r7 (raw file):

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

nit: The contract is actually:

fn create_store_key(
        event_type: &str,
        build_id: &str,
        invocation_id: &str,
        index: i64,
    ) -> StoreKey<'static>

Done.

---

nativelink-service/src/bep_server.rs line 61 at r7 (raw file):

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

nit: lol, now I would inline this, since you are not suffixing it with -L and it's self-explanatory now if inlined.

Done.

---

nativelink-service/tests/bep_server_test.rs line 199 at r7 (raw file):

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

This looks wrong. Why is it not passing the sequence number any more?

Reverted.

---

nativelink-service/tests/bep_server_test.rs line 298 at r7 (raw file):

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

nit: Why is this changed?

Reverted.

---

nativelink-store/src/redis_store.rs line 500 at r7 (raw file):

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

nit: try final_key.into() or &final_key (or see if it'll take it without conversion.

Done.

[allada]

Reviewed 1 of 7 files at r3, 3 of 3 files at r8, all commit messages.
Reviewable status: complete! 2 of 1 LGTMs obtained