Repository navigation
feat(storage): add ObjectTooLargeError and map it to EFBIG - #5193
PranjalC100 wants to merge 4 commits into
Conversation
gRPC parallel uploads reject writes beyond the maximum number of segments of an object with InvalidArgument. gcs.GetParallelUploadError returns that error as a new *gcs.ObjectTooLargeError, and the fs error mapping turns it into EFBIG instead of the default EIO. Nothing returns the error yet. The parallel upload writers will convert their errors with GetParallelUploadError, in Parallel write mode only.
![gemini-code-assist[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new ObjectTooLargeError to represent parallel upload failures when the maximum number of segments is exceeded, mapping it to syscall.EFBIG in the filesystem error wrapper. It also adds corresponding unit tests for the new error and its mapping. The review feedback suggests using status.FromError in GetParallelUploadError for consistency with other error-handling functions in the package when extracting gRPC status codes.
PranjalC100
added
execute-integration-tests
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5193 +/- ##
==========================================
+ Coverage 85.09% 85.11% +0.01%
==========================================
Files 179 179
Lines 19776 19787 +11
==========================================
+ Hits 16828 16841 +13
+ Misses 2947 2945 -2
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
the
remind-reviewers
Address review feedback on the ObjectTooLargeError and GetParallelUploadError doc comments.
InvalidArgument can be returned for other reasons as well. Track narrowing the check in GetParallelUploadError once the error for an object that is too large is known.
| if err == nil || errors.As(err, &objectTooLargeErr) { | ||
| return err | ||
| } | ||
| // TODO(b/578761925): InvalidArgument can be returned for other reasons as well. |
There was a problem hiding this comment.
i am not sure why we are adding this before the server errror is finalized. if we forget to do it or there is a delay in server implementation we would end up converting all invalid arguments to objectTooLarge error whereas the viceversa is fine
Description
Error handling for the parallel upload work (b/548851494), split out so it can be reviewed on its own. It doesn't depend on the other parallel upload PRs.
gcs.ObjectTooLargeError(internal/storage/gcs/errors.go): a generic error for an object that exceeds the maximum size supported by the upload strategy.gcs.GetParallelUploadError(err): converts anInvalidArgumenterror into*ObjectTooLargeError. nil, other errors, and an existing*ObjectTooLargeErrorare returned unchanged. Use it only for parallel uploads, because InvalidArgument means other things elsewhere.internal/fs/wrappers/error_mapping.go:*gcs.ObjectTooLargeError→EFBIGinstead of the defaultEIO. The fs error metrics already countEFBIG.Nothing returns the new error yet. A later PR calls
GetParallelUploadErroronwriter.Write,CreateObjectandFinalizeUploaderrors, in Parallel write mode only.Link to the issue in case of a bug fix.
b/577926767
Testing details
go test -race ./internal/storage/gcs/... ./internal/fs/wrappers/...: newTestGetParallelUploadError(6 cases),TestObjectTooLargeError_UnwrapsCause, andTestWithErrorMapping/TestObjectTooLargeError.Any backward incompatible change? If so, please explain.
No. Nothing returns the new error yet.