cleanup: return NotFound if blob stored w/o chunking#11395
cleanup: return NotFound if blob stored w/o chunking#11395tyler-french merged 1 commit intomasterfrom
Conversation
5c75238 to
576c8f2
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the implementation to align with a clarification in the Remote Execution API specification regarding the SplitBlob RPC. According to the updated spec, SplitBlob should return NOT_FOUND not only when a blob doesn't exist, but also when a blob exists but was stored without chunking metadata. The PR simplifies the code by removing logic that tried to distinguish between these cases and return different error codes.
Changes:
- Removed comment explaining the old behavior of returning
Unimplementedfor blobs stored without chunking - Simplified error handling in
LoadManifestto always return the sanitized error without checking if the blob exists - Removed unit test that verified
Unimplementederror was returned for blobs that exist but lack chunking metadata
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| server/remote_cache/content_addressable_storage_server/content_addressable_storage_server.go | Removed outdated TODO comment explaining the old behavior |
| server/remote_cache/chunking/chunking.go | Simplified LoadManifest error handling to return NOT_FOUND in all cases, removed errors import |
| server/remote_cache/chunking/chunking_test.go | Removed test that verified old behavior of returning Unimplemented for blobs without chunking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
576c8f2 to
f2379e9
Compare
f2379e9 to
5037fcf
Compare
maggie-lou
left a comment
There was a problem hiding this comment.
This change LGTM, but is there a world where we'd want the server to return the blob even if it is stored without chunking? Or is that unlikely to happen anyway once we're fully migrated to the CDC cache?
Yes, I'd like to experiment with on-demand chunking. Maybe even using an executor to split the blob. Added to https://github.com/buildbuddy-io/buildbuddy-internal/issues/6426 |
This PR cleans up the implementation to adhere to the improved spec, and replaces
Unimplementedwith a more accurateNotFounderror.bazelbuild/remote-apis@080cf12 (bazelbuild/remote-apis#353) clarified that the
SplitBlobserver can returnNOT_FOUNDif the blob exists but is stored without chunking.