cloud: fix gcs to resuming reader #66149
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
|
Pulled this commit out of #65967 since we don't plan on merging that yet. |
adityamaru
force-pushed
the
resuming-reader-fix
branch
from
e82cc6a to
8885390
Compare
dt
approved these changes
Jun 8, 2021
dt
reviewed
Jun 8, 2021
pkg/storage/cloud/cloud_io.go
Outdated
| return 0, errors.Wrap(lastErr, "multiple Read calls return no data") | ||
| // Check if the ResumingReader has been configured with retry-on-error | ||
| // decider. | ||
| if r.RetryOnErrFn != nil { |
There was a problem hiding this comment.
I wonder if we should log if RetryOnErrFn is nil (so we're not retrying) and/or fall back to a default function (e.g. sysutil.IsErrConnectionReset) since you could imagine forgetting to set RetryOnErrFn given we manually init the struct everywhere?
There was a problem hiding this comment.
Makes sense, I moved the struct creation to a NewResumingReader method to centralize the init.
adityamaru
force-pushed
the
resuming-reader-fix
branch
from
8885390 to
9a7dabc
Compare
This change does a few things: 1. gcs_storage was not returning a resuming reader as a result of which the Read method of the resuming reader that contains logic to retry on certain kinds of errors was not being invoked. 2, Changes the resuming reader to take a storage specific function that can define what errors are retryable in the resuming reader. All storage providers use the same deciding function at the moment and so behavior is unchanged. Release note: None
adityamaru
force-pushed
the
resuming-reader-fix
branch
from
9a7dabc to
22f6b33
Compare
|
TFTR! bors r=dt |
|
Build succeeded: |
adityamaru
added a commit
to adityamaru/cockroach
that referenced
this pull request
Jun 10, 2021
This change is a minimal backport of cockroachdb#66149. It fixes a bug where GCS was not using the resuming reader on Read(). Release note (bug fix): Fixes a bug where google cloud storage was not using the resuming reader, as a result of which some retryable errors were not being treated as such, and the Read would fail.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change does a few things:
which the Read method of the resuming reader that contains logic
to retry on certain kinds of errors was not being invoked.
2, Changes the resuming reader to take a storage specific function
that can define what errors are retryable in the resuming reader.
All storage providers use the same deciding function at the moment
and so behavior is unchanged.
Release note: None