Repository navigation
Conversation
| err := y.underlying.Restart(y.offsetReached) | ||
|
|
||
| if err != nil { | ||
| if storage.IsNonRetryableError(err) { |
| metrics.ReadReqErrors.Inc() | ||
| ylogger.Zero.Error().Err(err).Int64("offset reached", y.offsetReached).Int("bytes half-read", n).Int("retry count", int(retry)).Msg("encounter read error") | ||
| if err != nil { | ||
| if storage.IsNonRetryableError(err) { |
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/yezzey-gp/aws-sdk-go/aws/awserr" |
| ycl := mockcl.NewMockYproxyClient(ctrl) | ||
| yr := yio.NewYRetryReader(rr, ycl) | ||
|
|
||
| err507 := awserr.NewRequestFailure( |
| yr := yio.NewYRetryReader(rr, ycl) | ||
|
|
||
| err507 := awserr.NewRequestFailure( | ||
| awserr.New("InsufficientStorage", "object does not exist", nil), |
| ycl := mockcl.NewMockYproxyClient(ctrl) | ||
| yr := yio.NewYRetryReader(rr, ycl) | ||
|
|
||
| err507 := awserr.NewRequestFailure( |
| yr := yio.NewYRetryReader(rr, ycl) | ||
|
|
||
| err507 := awserr.NewRequestFailure( | ||
| awserr.New("InsufficientStorage", "object does not exist", nil), |
| * Storage returns 507 for objects that do not exist. | ||
| * Retrying such requests is pointless: the object will not appear. | ||
| */ | ||
| const nonRetryableStatusCode = 507 |
| */ | ||
| const nonRetryableStatusCode = 507 | ||
|
|
||
| func IsNonRetryableError(err error) bool { |
| func IsNonRetryableError(err error) bool { | ||
| var reqErr awserr.RequestFailure | ||
| if errors.As(err, &reqErr) { | ||
| return reqErr.StatusCode() == nonRetryableStatusCode |
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, 👼 SARIF report, or 📝 job summary for details.Unrecognized words (2)awserr These words are not needed and should be removedfuzz myminio ymlTo accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands... in a clone of the git@github.com:open-gpdb/yproxy.git repository curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/main/apply.pl' |
perl - 'https://github.com/open-gpdb/yproxy/actions/runs/38889638631/attempts/1' &&
git commit -m 'Update check-spelling metadata'OR To have the bot accept them for you, comment in the PR quoting the following line: Available 📚 dictionaries could cover words (expected and unrecognized) not in the 📘 dictionaryThis includes both expected items (287) from .github/actions/spelling/expect.txt and unrecognized words (2)
Consider adding them (in cspell:golang/dict/go.txt
cspell:node/dict/node.txt
cspell:php/dict/php.txt
cspell:python/src/python/python-lib.txt
cspell:dotnet/dict/dotnet.txt
cspell:fullstack/dict/fullstack.txt
cspell:java/src/java.txt
cspell:python/src/python/python.txt
cspell:typescript/dict/typescript.txt
cspell:k8s/dict/k8s.txt
cspell:filetypes/filetypes.txt
cspell:r/src/r.txt
cspell:django/dict/django.txt
cspell:aws/aws.txt
cspell:cpp/src/stdlib-c.txt
cspell:rust/dict/rust.txt
cspell:scala/dict/scala.txt
cspell:cpp/src/stdlib-cpp.txt
cspell:npm/dict/npm.txt
cspell:lua/dict/lua.txtTo stop checking additional dictionaries, add (in check_extra_dictionaries: ""Pattern suggestions ✂️ (13)You could add these patterns to Alternatively, if a pattern suggestion doesn't make sense for this project, add a Warnings and Notices
|
| Count | |
|---|---|
| ℹ️ candidate-pattern | 19 |
| 2 | |
| 1 | |
| ℹ️ unused-config-file | 1 |
See
If the flagged items are 🤯 false positives
If items relate to a ...
-
binary file (or some other file you wouldn't want to check at all).
Please add a file path to the
excludes.txtfile matching the containing file.File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
^refers to the file's path from the root of the repository, so^README\.md$would exclude README.md (on whichever branch you're using). -
well-formed pattern.
If you can write a pattern that would match it,
try adding it to thepatterns.txtfile.Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.
Note that patterns can't match multiline strings.
🚂 If you're seeing this message and your PR is from a branch that doesn't have check-spelling,
please merge to your PR's base branch to get the version configured for your repository.
There was a problem hiding this comment.
🟡 Changes recommended
Both new error paths return negative byte counts, violating the reader contract and potentially panicking buffered consumers.
2 open findings
What changed in this PR
Stops storage read retries immediately on HTTP 507 errors.
Changes:
- Adds a helper to identify non-retryable 507 errors.
- Applies early returns during restart and read operations.
- Adds tests for both failure paths.
| File | Description |
|---|---|
| pkg/storage/utils.go | Classifies 507 request failures. |
| pkg/proc/yio/yrreader.go | Stops retries on classified errors. |
| pkg/proc/yio/yrreader_test.go | Tests immediate failure without retries. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if err != nil { | ||
| if storage.IsNonRetryableError(err) { | ||
| ylogger.Zero.Error().Err(err).Int64("offset reached", y.offsetReached).Msg("object does not exist in external storage, giving up") | ||
| return -1, err |
There was a problem hiding this comment.
I agree
Read reads up to len(p) bytes into p. It returns the number of bytes read (0 <= n <= len(p)) and any error encountered.
Source: https://pkg.go.dev/io#Reader
| if err != nil { | ||
| if storage.IsNonRetryableError(err) { | ||
| ylogger.Zero.Error().Err(err).Int64("offset reached", y.offsetReached).Msg("object does not exist in external storage, giving up") | ||
| return -1, err |
|
I think to need addition to description 507 code is not standard, but contract from s3 (s3 use free code and assign a meaning) |
| yr := yio.NewYRetryReader(rr, ycl) | ||
|
|
||
| err507 := awserr.NewRequestFailure( | ||
| awserr.New("InsufficientStorage", "object does not exist", nil), |
There was a problem hiding this comment.
Message incorrect. Return code from s3 use other semantic not equal http standard.
| const nonRetryableStatusCode = 507 | ||
|
|
||
| func IsNonRetryableError(err error) bool { | ||
| var reqErr awserr.RequestFailure | ||
| if errors.As(err, &reqErr) { | ||
| return reqErr.StatusCode() == nonRetryableStatusCode | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Name IsNonRetryableError has broad meaning, but return code means "object does not exists for sure". We use const only into function I think we need const move to function body.
| const nonRetryableStatusCode = 507 | |
| func IsNonRetryableError(err error) bool { | |
| var reqErr awserr.RequestFailure | |
| if errors.As(err, &reqErr) { | |
| return reqErr.StatusCode() == nonRetryableStatusCode | |
| } | |
| return false | |
| } | |
| func IsMissingObjectError(err error) bool { | |
| const missingObjectStatusCode = 507 | |
| var reqErr awserr.RequestFailure | |
| return errors.As(err, &reqErr) && | |
| reqErr.StatusCode() == missingObjectStatusCode | |
| } |
There was a problem hiding this comment.
Maybe without const better. We see code number and function name, and know s3 contract.
| const nonRetryableStatusCode = 507 | |
| func IsNonRetryableError(err error) bool { | |
| var reqErr awserr.RequestFailure | |
| if errors.As(err, &reqErr) { | |
| return reqErr.StatusCode() == nonRetryableStatusCode | |
| } | |
| return false | |
| } | |
| func IsMissingObjectError(err error) bool { | |
| var reqErr awserr.RequestFailure | |
| return errors.As(err, &reqErr) && | |
| reqErr.StatusCode() == 507 | |
| } |
| 507, "request-id") | ||
|
|
||
| /* no retry on 507: single Restart attempt, then error is returned */ | ||
| rr.EXPECT().Restart(int64(0)).Return(err507).Times(1) |
There was a problem hiding this comment.
Maybe we want to compare the case with a repeat and without a repeat, but with error.

507 from S3 means object does not exists for sure.
So there are no need to read it - break immidiately and return error.