FrankChen021 commented on code in PR #20806:
URL: https://github.com/apache/druid/pull/20806#discussion_r4434810174
##########
processing/src/main/java/org/apache/druid/segment/loading/DeepStorageSegmentConfig.java:
##########
@@ -40,34 +43,59 @@
*/
public class DeepStorageSegmentConfig
{
- @JsonProperty
+ private static final Logger log = new Logger(DeepStorageSegmentConfig.class);
+
+ @JsonProperty("compressionFormat")
@Nullable
- private final Boolean zip;
+ private final CompressionUtils.Format compressionFormat;
@JsonCreator
- public DeepStorageSegmentConfig(@JsonProperty("zip") @Nullable Boolean zip)
+ public DeepStorageSegmentConfig(
+ @JsonProperty("zip") @Nullable Boolean zip,
+ @JsonProperty("compressionFormat") @Nullable CompressionUtils.Format
compressionFormat
+ )
+ {
+ if (zip != null) {
+ log.warn("druid.storage.zip is deprecated, use
druid.storage.compressionFormat instead.");
+ }
+ if (compressionFormat != null) {
+ this.compressionFormat = compressionFormat;
+ } else if (zip != null) {
+ this.compressionFormat = zip ? CompressionUtils.Format.ZIP :
CompressionUtils.Format.NONE;
+ } else {
+ this.compressionFormat = null;
+ }
+ }
+
+ public DeepStorageSegmentConfig(@Nullable Boolean zip)
+ {
+ this(zip, null);
+ }
+
+ public DeepStorageSegmentConfig(@Nullable CompressionUtils.Format
compressionFormat)
{
- this.zip = zip;
+ this(null, compressionFormat);
}
public DeepStorageSegmentConfig()
{
- this(null);
+ this(null, null);
}
/**
- * Whether segments should be written as a zip file, falling back to {@code
defaultZip} when
- * {@code druid.storage.zip} has not been set.
+ * Resolves the effective compression format to use, falling back to {@code
defaultFormat} when neither
+ * {@code druid.storage.compressionFormat} nor {@code druid.storage.zip} has
been set.
*/
- public boolean isZip(boolean defaultZip)
+ public CompressionUtils.Format
getEffectiveCompressionFormat(CompressionUtils.Format defaultFormat)
Review Comment:
[P1] Keep the deprecated DeepStorageSegmentConfig API
**Finding:** The latest commit removes the public isZip(boolean)
compatibility shim and getZip() accessor from this configuration class.
Downstream extensions or integrations compiled against the previous API now
fail at compile or link time, contradicting the PR's stated requirement to
retain isZip(boolean) for Java API compatibility while the legacy zip property
remains supported.
**Suggestion:** Restore both methods as deprecated adapters backed by the
effective compression format, and preserve the existing legacy
constructor/source signature while the zip property remains supported.
##########
extensions-core/hdfs-storage/src/main/java/org/apache/druid/storage/hdfs/HdfsDataSegmentKiller.java:
##########
@@ -74,52 +86,72 @@ public void kill(DataSegment segment) throws
SegmentLoadingException
try (final FileSystem fs = segmentPath.getFileSystem(config)) {
final String filename = segmentPath.getName();
final CompressionUtils.Format compressionFormat =
CompressionUtils.Format.fromFileName(filename);
- if (compressionFormat != CompressionUtils.Format.ZIP &&
compressionFormat != CompressionUtils.Format.LZ4) {
+
+ final String nameWithoutExtension = compressionFormat == null
+ ? filename
+ : filename.substring(0, filename.length() -
compressionFormat.getSuffix().length());
+
+ // Validate the file name format before checking if it exists.
+ // This ensures we throw SegmentLoadingException for invalid names
+ // even if they do not exist on the filesystem, satisfying tests.
+ if (!isUncompressedDirectorySegment(nameWithoutExtension)
Review Comment:
[P2] Do not delete unsupported-extension directories
**Finding:** For a path such as 0_index.gz, fromFileName returns GZ and
nameWithoutExtension becomes 0_index, so this validation accepts it as an
uncompressed directory. If that path exists as a directory, the later branch
permits it and recursively deletes it even though HDFS supports only ZIP/LZ4
files or extensionless directories; a malformed loadSpec can therefore delete
an unrelated directory that the previous killer rejected.
**Suggestion:** Only classify a directory as uncompressed when the original
name has no compression extension or an explicitly supported ZIP/LZ4 extension
used by the NONE compatibility path, and reject all other extensions before
deletion.
##########
processing/src/main/java/org/apache/druid/segment/loading/DeepStorageSegmentConfig.java:
##########
@@ -40,34 +43,59 @@
*/
public class DeepStorageSegmentConfig
{
- @JsonProperty
+ private static final Logger log = new Logger(DeepStorageSegmentConfig.class);
+
+ @JsonProperty("compressionFormat")
@Nullable
- private final Boolean zip;
+ private final CompressionUtils.Format compressionFormat;
@JsonCreator
- public DeepStorageSegmentConfig(@JsonProperty("zip") @Nullable Boolean zip)
+ public DeepStorageSegmentConfig(
+ @JsonProperty("zip") @Nullable Boolean zip,
+ @JsonProperty("compressionFormat") @Nullable CompressionUtils.Format
compressionFormat
+ )
+ {
+ if (zip != null) {
+ log.warn("druid.storage.zip is deprecated, use
druid.storage.compressionFormat instead.");
+ }
+ if (compressionFormat != null) {
Review Comment:
[P2] Reject conflicting compression properties
**Finding:** When both druid.storage.zip and druid.storage.compressionFormat
are set to incompatible values, this constructor now silently prefers
compressionFormat after logging only a deprecation warning. A configuration
such as zip=true plus compressionFormat=none therefore starts and changes the
segment layout instead of failing validation, making migration typos or
conflicting rollouts invisible.
**Suggestion:** Restore validation for incompatible legacy/new pairs and
fail configuration before a pusher is initialized.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
