Fix/panic in GETRANGE
#2101
Fix/panic in GETRANGE
#2101
Conversation
carpawell
requested review from
alexvanin,
fyrchik and
cthulhu-rider
as code owners
|
I have recieved a notification of a new pull request. I am starting the build of images and binaries for further testing. Build number is 3712 |
|
I have recieved a notification of a new pull request. I am starting the build of images and binaries for further testing. Build number is 3713 |
|
I am running integration tests |
|
Test run is finished. Please download the tarball from link. Untar and use |
|
I have recieved a notification of a new pull request. I am starting the build of images and binaries for further testing. Build number is 3714 |
|
I am running integration tests |
Codecov Report
@@ Coverage Diff @@
## master #2101 +/- ##
==========================================
- Coverage 30.78% 30.77% -0.01%
==========================================
Files 382 382
Lines 28056 28065 +9
==========================================
Hits 8637 8637
- Misses 18687 18696 +9
Partials 732 732
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Test run is finished. Please download the tarball from link. Untar and use |
|
I have recieved a notification of a new pull request. I am starting the build of images and binaries for further testing. Build number is 3731 |
|
Changed the base branch. |
|
I am running integration tests |
|
I have recieved a notification of a new pull request. I am starting the build of images and binaries for further testing. Build number is 3740 |
|
I am running integration tests |
|
Test run is finished. Please download the tarball from link. Untar and use |
| } | ||
|
|
||
| if offset+length <= offset { | ||
| return nil, fmt.Errorf("invalid '%s' range: uint64 overflow", vs[i]) |
There was a problem hiding this comment.
Could you also check the test behaviour? They may check for out of range error.
There was a problem hiding this comment.
seems like tests want OUT_OF_RANGE here /cc @abereziny
should be adapted then
There was a problem hiding this comment.
It's okay to change. We may update testcases to reflect this new error
|
I have recieved a notification of a new pull request. I am starting the build of images and binaries for further testing. Build number is 3771 |
|
I am running integration tests |
|
Test run is finished. Please download the tarball from link. Untar and use |
| } | ||
|
|
||
| if offset+length <= offset { | ||
| return nil, apistatus.ObjectOutOfRange{} |
There was a problem hiding this comment.
To me this is not a status error even though it probably will.
There was a problem hiding this comment.
I agree, can we print the text, but make sure user understands this is a local error.
There was a problem hiding this comment.
yes, i agree but that is what our tests expect. we can look at this as an optimisation, a node should return that code. anyway SDK client panics in that case so we have to rerurn some error here. @fyrchik
There was a problem hiding this comment.
so what have we decided?
There was a problem hiding this comment.
make sure user understands this is a local error
we write rpc error: to all the RPC errors we get
There was a problem hiding this comment.
returned the previous version (not the status code) and suggest changing the test in its failure case
| } | ||
|
|
||
| if offset+length <= offset { | ||
| return nil, apistatus.ObjectOutOfRange{} |
There was a problem hiding this comment.
I agree, can we print the text, but make sure user understands this is a local error.
Also, includes range parsing error messages enhancement. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Stop child objects collection if the last returned object (the most "left" object in the collected chain) starts exactly from the `GETRANGE`'s `from` value. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
|
Test run is finished. Please download the tarball from link. Untar and use |
Closes #2095.