perf: Streamline s3 backend#19394
Conversation
180aa0f to
4986d81
Compare
4986d81 to
50af918
Compare
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 8 of 8 changed files.
This is an automated review by Codex GPT-5.5
| sourceMetadata = s3Client.getObjectMetadata(s3Bucket, s3Path); | ||
| } | ||
| catch (S3Exception e) { | ||
| if (e.statusCode() == 404 && "NoSuchKey".equals(S3Utils.getS3ErrorCode(e))) { |
There was a problem hiding this comment.
[P2] Treat any HEAD 404 as a missing source
The idempotent already-moved path now only runs when headObject fails with status 404 and error code NoSuchKey. S3 HEAD failures for absent objects can surface as a generic 404/NotFound response, and other Druid S3 code handles missing HEAD results by status alone. In that case this code rethrows before checking whether the target object already exists, so a retry or competing move where the source was deleted after a successful copy can incorrectly fail instead of returning the moved segment. Please key this fallback off statusCode() == 404 rather than requiring NoSuchKey.
Description
S3 achieve strong read-after-write consistency in 2020. The current s3 backend architecture assumes a prior consistency model and therefore does some redundant calls which are both slow and costly.
Some other things to look into in a separate PR is parallel download requests using byte ranges for a single file (currently we use a single TCP connection which isn't the fastest and is subject to S3 single-connection bandwidth limitations).
High-level list of changes
isObjectInBucketguards before zip and gzip downloads (and the now-dead private method). The 404 from GetObject propagates as aSegmentLoadingException.doesObjectExist+listObjectsV2two-call sequence with a singlegetObjectMetadatarequest inS3DataSegmentMover.Release note
This PR has: