feat: add support for push/pull/kill segments in google cloud deep storage without zip compression

[clintropolis]

Description

Follow-up to #20327 to do the same thing for google cloud storage.

[FrankChen021]

Two things that I would like to ask for clarification:

In this PR(and other object storage like deep storage), the new propertydruid.storage.zip is used to determine whether segment files are compressed as index.zip or not. However, in #18982 , we introduced a new property druid.storage.compressionFormat for HDFS deep storage to indicate which compression format is used for compression(zip and lz4), that PR didn't bring the change to object storage like S3, Azure ang google cloud(because we don't use object storage), but architecturally, that property should apply to all deep storages. So I think we should use the druid.storage.compressionFormat( with a new NONE enum supported ) to replace the druid.storage.zip.

Secondly, do you have any data that shows the segment size without compression as index.zip in the deep storage?
As commented in #18982, zip 0 (which does not comrpess but only pack files together) has a size of 11GiB, while the default zip level currently we are using now shows the size is about 2.48GB, this means that if segments are not compressed, file size in deep storage should be much larger. I'm not sure if such file size increasing is consistent in your case.

[FrankChen021]

🟡 Changes recommended

Uploading an unzipped segment directly into its final GCS prefix makes replacement observable before all files are present. A concurrent reload or same-path rewrite can therefore materialize a mixed segment, so this should be addressed before enabling the new layout.

Reviewed 9 of 9 changed files; finding in 1 file.

Severity	Findings
P0	0
P1	1
P2	0
P3	0
Total	1

Findings that could not be attached inline:

• extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleDataSegmentPusher.java:174 - [P1] Unzipped replacement is not atomic

  Finding: When druid.storage.zip=false, pushNoZip uploads each file directly under the final segment prefix and only then lists and deletes stale objects. If a historical or another pusher reads the same path during a retry or same-path rewrite, it can observe missing new files, old chunks, or a mixture of two segment versions; the puller can then materialize a corrupt or incomplete segment even though the push later succeeds.

  Suggestion: Stage the complete file set under an isolated generation and publish it through an atomic marker or path switch, or otherwise prevent readers from seeing the prefix until upload and cleanup are complete.

---

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.

[clintropolis]

In this PR(and other object storage like deep storage), the new property druid.storage.zip is used to determine whether segment files are compressed as index.zip or not. However, in #18982 , we introduced a new property druid.storage.compressionFormat for HDFS deep storage to indicate which compression format is used for compression(zip and lz4), that PR didn't bring the change to object storage like S3, Azure ang google cloud(because we don't use object storage), but architecturally, that property should apply to all deep storages. So I think we should use the druid.storage.compressionFormat( with a new NONE enum supported ) to replace the druid.storage.zip.

My stance is that it would make sense to add druid.storage.compressionFormat to DeepStorageSegmentConfig, but I didn't personally have much interest in adding support for it to the other deep storage implementations because I am primarily interested in storing V10 segment format files in deep storage uncompressed so that SegmentRangeReader can do partial reads to support partial loads on historicals. V10 format stores the segment as a single file (metadata header + a bunch of concatenated internal files so kind similar in spirit to zip 0) so it shouldn't change the overall number of files in deep storage (at least without any extensions to add/attach 'external' files).

In terms of compression/encoding, going forward I am much more interested in ways to shrink the internal files stored inside the v10 segment file so that we can preserve the ability to do partial reads, so I'll be spending any time/energy I have to pursue that instead of expanding the types of generic compression we can apply to the whole container

Secondly, do you have any data that shows the segment size without compression as index.zip in the deep storage?
As #18982 (comment), zip 0 (which does not comrpess but only pack files together) has a size of 11GiB, while the default zip level currently we are using now shows the size is about 2.48GB, this means that if segments are not compressed, file size in deep storage should be much larger. I'm not sure if such file size increasing is consistent in your case.

I don't have any hard data on this, in part because it doesn't matter because of the need to be able to do partial reads and I haven't had much time to do analysis of what we could improve inside the segment (because external compression is basically not an option and size hasn't proven problematic yet). 11GiB is quite a lot larger than most of the segments we deal with, I'm still fairly interested in the composition of the segment in that example to find opportunities to compress the internal files (i theorized at the time in that PR that perhaps a lot of uncompressed complex types were making the difference between compressed and uncompressed so different).

[clintropolis]

Finding: When druid.storage.zip=false, pushNoZip uploads each file directly under the final segment prefix and only then lists and deletes stale objects. If a historical or another pusher reads the same path during a retry or same-path rewrite, it can observe missing new files, old chunks, or a mixture of two segment versions; the puller can then materialize a corrupt or incomplete segment even though the push later succeeds.

I don't think this is really a problem, is it? Historicals won't ever be pulling a segment that didn't publish successfully (load spec doesn't exist in metadata until pushes all suceed), ingest time replicas I don't believe will publish at the same time (and they should be producing identical contents), and the only real replace case I'm aware of where content could be different is like when streaming failed to publish and has to retry and while starting at the same offsets, might end at different offsets, but that runs with useUniquePath=true so avoid the problem.

[FrankChen021]

Finding: When druid.storage.zip=false, pushNoZip uploads each file directly under the final segment prefix and only then lists and deletes stale objects. If a historical or another pusher reads the same path during a retry or same-path rewrite, it can observe missing new files, old chunks, or a mixture of two segment versions; the puller can then materialize a corrupt or incomplete segment even though the push later succeeds.

I don't think this is really a problem, is it? Historicals won't ever be pulling a segment that didn't publish successfully (load spec doesn't exist in metadata until pushes all suceed), ingest time replicas I don't believe will publish at the same time (and they should be producing identical contents), and the only real replace case I'm aware of where content could be different is like when streaming failed to publish and has to retry and while starting at the same offsets, might end at different offsets, but that runs with useUniquePath=true so avoid the problem.

You're right. Sometimes AI does not correctly understand the business flow.

[FrankChen021]

In this PR(and other object storage like deep storage), the new property druid.storage.zip is used to determine whether segment files are compressed as index.zip or not. However, in #18982 , we introduced a new property druid.storage.compressionFormat for HDFS deep storage to indicate which compression format is used for compression(zip and lz4), that PR didn't bring the change to object storage like S3, Azure ang google cloud(because we don't use object storage), but architecturally, that property should apply to all deep storages. So I think we should use the druid.storage.compressionFormat( with a new NONE enum supported ) to replace the druid.storage.zip.

My stance is that it would make sense to add druid.storage.compressionFormat to DeepStorageSegmentConfig, but I didn't personally have much interest in adding support for it to the other deep storage implementations because I am primarily interested in storing V10 segment format files in deep storage uncompressed so that SegmentRangeReader can do partial reads to support partial loads on historicals. V10 format stores the segment as a single file (metadata header + a bunch of concatenated internal files so kind similar in spirit to zip 0) so it shouldn't change the overall number of files in deep storage (at least without any extensions to add/attach 'external' files).

In terms of compression/encoding, going forward I am much more interested in ways to shrink the internal files stored inside the v10 segment file so that we can preserve the ability to do partial reads, so I'll be spending any time/energy I have to pursue that instead of expanding the types of generic compression we can apply to the whole container

I agree with you that as you're putting effort on the new segment file format, the new compression does not make sense, we also don't have any plan/interest to push these forward.
To make current configuration design consistency across all deep storages, for this part, my position is that we can do a small clean up to deprecate the druid.storage.zip and use druid.storage.compressionFormat instead to eliminate ambigulty. If you don't have time, I can do this after this is merged.

[clintropolis]

I agree with you that as you're putting effort on the new segment file format, the new compression does not make sense, we also don't have any plan/interest to push these forward.
To make current configuration design consistency across all deep storages, for this part, my position is that we can do a small clean up to deprecate the druid.storage.zip and use druid.storage.compressionFormat instead to eliminate ambigulty. If you don't have time, I can do this after this is merged.

I re-read my last comment and it maybe came across a bit stronger than I intended 😅 , I'm certainly not opposed to wiring up those formats, and i think its probably fine if we deprecate druid.storage.zip in favor of druid.storage.compressionFormat, was mainly just trying to explain/defend why i avoided doing it in this or the last PR since the on/off toggle is a bit smaller than wiring up all of the formats. I can also potentially look into this after this goes in to unblock adding support for partial loads to google cloud storage.

[capistrant]

feels like an odd exception type for a failed delete, but I see there is precedent for using it like this in S3DataSegmentKiller so I will not object outright