Repository navigation
feat: add support to push/pull/kill segments in azure deep storage without zip compression - #20327
Conversation
…thout zip compression
github-actions
Bot
added
Area - Documentation
Area - Batch Ingestion
Area - Segment Format and Ser/De
Area - Ingestion
Area - MSQ
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
The Azure unzipped layout is functionally wired through push, pull, and kill, but the destination replacement and failed-download paths need correction before this mode is safe for production segment publication and loading. The P1 finding is the merge blocker; the P2 finding affects retry/fallback behavior and partial-cache cleanup.
Reviewed 27 of 27 changed files.
Validation was limited to static inspection and git diff --check, which passed. I did not run prepare, fetch, cleanup, builds, tests, dependency installation, or formatters.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
| } | ||
|
|
||
| long size = 0; | ||
| for (final File file : files) { |
There was a problem hiding this comment.
[P1] Replace stale blobs when overwriting an unzipped segment
When druid.storage.zip=false, this loop overwrites only files present in the new segment and never removes objects already under azureBasePath (including a prior index.zip or old smoosh files). push is explicitly allowed to run with useUniquePath=false—BatchAppenderator does this—and the DataSegmentPusher contract requires that mode to replace a previous push. A retry or replica with a different file set therefore leaves stale blobs; overlapping pushes can also interleave per-file overwrites. The directory puller lists the whole prefix, so it can load a mixed or corrupt segment. Stage to an isolated prefix and publish/replace safely, or otherwise guarantee stale destination blobs cannot be visible.
| final ByteSource byteSource = byteSourceFactory.create(containerName, blobPath, azureStorage); | ||
| final File outFile = new File(outDir, Paths.get(blobPath).getFileName().toString()); | ||
| copyResult.addFiles( | ||
| FileUtils.retryCopy(byteSource, outFile, AzureUtils.AZURE_RETRY, maxTries).getFiles() |
There was a problem hiding this comment.
[P2] Preserve pull failure cleanup and error handling
FileUtils.retryCopy wraps its final checked I/O failure in RuntimeException, while getSegmentFiles only catches IOException (lines 84-96). A failed retry for any blob in this new directory path therefore escapes as a runtime exception and bypasses the cleanup that removes outDir; after earlier blobs were copied, the caller can be left with a partial segment. It also bypasses the SegmentLoadingException contract used by AzureLoadSpec and prevents SegmentLocalCacheManager from trying another storage location. Preserve or translate the checked failure here so the existing cleanup and fallback path apply.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
The prior review's Azure stale-destination and failed-download cleanup findings are addressed in the current head. One S3-specific regression remains: the new unzipped stale-object cleanup passes the pusher config's default listing size of 1024 into the AWS listing/deletion helper, which is limited to 1000 keys. This can make every default-configured S3 unzipped push fail after uploading its files and leave the partial replacement in storage.
Reviewed 27 of 27 changed files.
Validation was limited to static inspection and git diff --check, which passed. I did not run builds, tests, dependency installation, formatters, prepare, fetch, or cleanup.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
| try { | ||
| S3Utils.deleteObjectsInPath( | ||
| s3Client, | ||
| config.getMaxListingLength(), |
There was a problem hiding this comment.
[P1] S3 cleanup uses an invalid default batch size
Finding: The new stale-object cleanup passes S3DataSegmentPusherConfig's maxListingLength directly to S3Utils.deleteObjectsInPath. That config defaults to 1024 and only enforces a lower bound, while this helper uses the value for both ListObjectsV2 maxKeys and DeleteObjects batches; AWS S3 permits at most 1000 keys for either request. Consequently, a default-configured druid.storage.zip=false push can upload the files and then fail cleanup before returning its DataSegment.
Suggestion: Cap the cleanup/listing size at 1000 or apply the same upper-bound validation and default used by S3InputDataConfig, and add a regression test with the default pusher configuration.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
Approval is recommended. The current head addresses the prior review findings: Azure unzipped pushes remove stale destination blobs, Azure failed downloads clean the output directory, and S3 stale-object deletion now caps each DeleteObjects request at the 1,000-key service limit while continuing to paginate listings. No additional high-confidence, PR-caused correctness, lifecycle, concurrency, security, compatibility, data-loss, or missing-test issue was found.
Exact coverage: reviewed all 29 of 29 changed files: docs/development/extensions-core/azure.md; docs/development/extensions-core/s3.md; extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureDataSegmentKiller.java; extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureDataSegmentPuller.java; extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureDataSegmentPusher.java; extensions-core/azure-extensions/src/test/java/org/apache/druid/storage/azure/AzureDataSegmentKillerTest.java; extensions-core/azure-extensions/src/test/java/org/apache/druid/storage/azure/AzureDataSegmentPullerTest.java; extensions-core/azure-extensions/src/test/java/org/apache/druid/storage/azure/AzureDataSegmentPusherTest.java; extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentKiller.java; extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentPusher.java; extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentPusherConfig.java; extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3Utils.java; extensions-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/S3DataSegmentPusherConfigTest.java; extensions-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/S3DataSegmentPusherTest.java; indexing-service/src/test/java/org/apache/druid/indexing/common/task/CompactionTaskRunBase.java; indexing-service/src/test/java/org/apache/druid/indexing/common/task/IngestionTestBase.java; indexing-service/src/test/java/org/apache/druid/indexing/common/task/batch/parallel/AbstractParallelIndexSupervisorTaskTest.java; indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskTestBase.java; indexing-service/src/test/java/org/apache/druid/indexing/worker/shuffle/ShuffleDataSegmentPusherTest.java; multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQCompactionTaskRunTest.java; multi-stage-query/src/test/java/org/apache/druid/msq/test/CalciteMSQTestsHelper.java; multi-stage-query/src/test/java/org/apache/druid/msq/test/MSQTestBase.java; processing/src/main/java/org/apache/druid/segment/loading/DeepStorageSegmentConfig.java; processing/src/test/java/org/apache/druid/segment/loading/DeepStorageSegmentConfigTest.java; server/src/main/java/org/apache/druid/guice/LocalDataStorageDruidModule.java; server/src/main/java/org/apache/druid/segment/loading/LocalDataSegmentPusher.java; server/src/main/java/org/apache/druid/segment/loading/LocalDataSegmentPusherConfig.java; server/src/test/java/org/apache/druid/guice/LocalDataStorageDruidModuleTest.java; server/src/test/java/org/apache/druid/segment/loading/LocalDataSegmentPusherTest.java.
Validation: git diff --check passed against the prepared worktree. No builds, tests, dependency installs, formatters, preparation, fetches, or cleanup were run.
This is an automated review by Codex GPT-5.6-Luna(max)
capistrant
left a comment
capistrant
left a comment
There was a problem hiding this comment.
looks good to me. Only doubt is if we need to surface the new authz requirement for unzipped segments in any way other than in the config table like you have done? @317brian ?
4 participants
Description
Updates Azure deep storage extension to support pushing/pulling/killing segments without zip compression (
druid.storage.zip). In the process, I refactored the other deep storage implementations that supportdruid.storage.zip(local and s3) to move the config to a newDeepStorageSegmentConfigclass to be shared by all deep storage implementations that is bound todruid.storageand available to inject to all the pushers/pullers/killers. Currently it only has the zip property, but anything else that is specific to how segments are laid out in deep storage can live here eventually as well.I plan to wire up google cloud storage as a follow-up PR.