Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(storage): propagate ctx from invoke to grpc upload reqs #11475

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BrennaEpp
Copy link
Contributor

The context from run (see invoke.go) is not being used for gRPC uploads.

Rather than keep a source of truth in a writer var (except for the original context), pass through contexts.

The other option is to keep as is and instead do the following. This may be "safer" (at least in terms of, more likely to keep current behaviour) as it is hard to follow exactly how these contexts are behaving currently.

                                 uploadBuff := func(ctx context.Context) error {
					saveCtx := gw.ctx
					gw.ctx = ctx
					obj, err := gw.uploadBuffer(ctx, recvd, offset, doneReading)
					o = obj
					gw.ctx = saveCtx
					return err
				}
  • Note: bucketContext also changed to be called once for ALL grpc upload types.

@BrennaEpp BrennaEpp requested review from a team as code owners January 21, 2025 01:02
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Jan 21, 2025
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjc25 I think this looks like a good change but just FYI.

@cjc25
Copy link
Contributor

cjc25 commented Jan 21, 2025

@cjc25 I think this looks like a good change but just FYI.

LGTM. The stashed context was mirroring the implementation as of Nov 2024, which also stashed a context (and also didn't use run):

ctx context.Context
. I think @BrennaEpp is right that the prior behavior didn't handle retry/backoff correctly. I did not merge #11200 correctly, which is how we lost it.

@tritone
Copy link
Contributor

tritone commented Jan 21, 2025

@cjc25 I think this looks like a good change but just FYI.

LGTM. The stashed context was mirroring the implementation as of Nov 2024, which also stashed a context (and also didn't use run):

ctx context.Context

. I think @BrennaEpp is right that the prior behavior didn't handle retry/backoff correctly. I did not merge #11200 correctly, which is how we lost it.

Re: #11200 , that was expected, @BrennaEpp re-implemented the retry/backoff behavior in the private repo given the large changes in uploads overall.

@BrennaEpp
Copy link
Contributor Author

@cjc25 I think this looks like a good change but just FYI.

LGTM. The stashed context was mirroring the implementation as of Nov 2024, which also stashed a context (and also didn't use run):

ctx context.Context

. I think @BrennaEpp is right that the prior behavior didn't handle retry/backoff correctly. I did not merge #11200 correctly, which is how we lost it.

Re: #11200 , that was expected, @BrennaEpp re-implemented the retry/backoff behavior in the private repo given the large changes in uploads overall.

+1, when I introduced run to get the backoff I didn't think about the ctx from run being lost; really, it has never used that ctx and this change adds it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants