GrpcStore retry first

[chrisstaite-menlo]

Description

We see the first request in a GrpcStore request failing but this bypasses the retry logic because it's already passed out of the retry function.

Read the first message from the upstream GrpcStore within the retry logic, only exit from the retry logic after the first message was recieved.

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please also list any relevant details for your test configuration

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 

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nativelink-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 19, 2024 10:40am

[chrisstaite-menlo]

This was an attempt to fix #610 however, it doesn't appear to have made any difference.

[allada]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04

---

nativelink-store/src/grpc_store.rs line 68 at r1 (raw file):

}

struct FirstStream {

nit: Lets add some documentation above this on why we read the first stream item and how this helps.

---

nativelink-store/src/grpc_store.rs line 672 at r1 (raw file):

        loop {
            let message = match future::poll_fn(|cx| Pin::new(&mut stream).poll_next(cx)).await {

nit: You should be able to import futures::StreamExt then call stream.next().await

---

nativelink-util/src/buf_channel.rs line 167 at r1 (raw file):

        let maybe_chunk = match self.partial.take() {
            // Only return bytes if we haven't already reached EOF.
            Some(result_bytes) => self.close_tx.is_some().then_some(result_bytes),

Oh interesting.

Lets reword this comment:

// `partial` is allowed to have empty bytes that represent EOF, but `self.rx.recv()` should never
// respond with empty bytes as EOF. If the `partial` is an empty bytes pass None to simulate
// the stream's version of EOF.

---

nativelink-util/src/buf_channel.rs line 167 at r1 (raw file):

        let maybe_chunk = match self.partial.take() {
            // Only return bytes if we haven't already reached EOF.
            Some(result_bytes) => self.close_tx.is_some().then_some(result_bytes),

Do we really want to check self.close_tx? Shouldn't we just check if result_bytes is empty?

[chrisstaite-menlo]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04

---

nativelink-util/src/buf_channel.rs line 167 at r1 (raw file):

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

Oh interesting.

Lets reword this comment:

// `partial` is allowed to have empty bytes that represent EOF, but `self.rx.recv()` should never
// respond with empty bytes as EOF. If the `partial` is an empty bytes pass None to simulate
// the stream's version of EOF.

Done.

---

nativelink-util/src/buf_channel.rs line 167 at r1 (raw file):

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

Do we really want to check self.close_tx? Shouldn't we just check if result_bytes is empty?

Done.

[adam-singer]

Reviewed 1 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained

---

nativelink-store/src/grpc_store.rs line 72 at r2 (raw file):

/// the connection establishes fine, but reading the first byte of the file
/// fails we have the ability to retry before returning to the caller.
struct FirstStream {

nit: wondering out loud if this kind of trick would have been needed if we "warm up" clients, meaning before service is fully ready we do some no-op operations across clients so machinery / connections are established?
Does this kind of stream read failure happen regardless even if clients are warn/connected?

[chrisstaite-menlo]

nativelink-store/src/grpc_store.rs line 72 at r2 (raw file):

Previously, adam-singer (Adam Singer) wrote…

nit: wondering out loud if this kind of trick would have been needed if we "warm up" clients, meaning before service is fully ready we do some no-op operations across clients so machinery / connections are established?
Does this kind of stream read failure happen regardless even if clients are warn/connected?

I'm not really sure, I haven't had the issue since the file store fix has been put in place, but having said that I'm also running this code too, so maybe I have?

I am still having horrid issues with a GoAway frame being sent by the upstream store which then appears to drop all requests to that store. I think we need to add a limit to the number of concurrent upstream GrpcStore requests we perform to avoid this.

[allada]

Sorry for delay.

Reviewable status: 2 of 1 LGTMs obtained