FrankChen021 commented on code in PR #20806:
URL: https://github.com/apache/druid/pull/20806#discussion_r4429583444
##########
extensions-core/hdfs-storage/src/main/java/org/apache/druid/storage/hdfs/HdfsDataSegmentPusher.java:
##########
@@ -123,15 +156,16 @@ public DataSegment push(final File inDir, final
DataSegment segment, final boole
public DataSegment pushToPath(File inDir, DataSegment segment, String
storageDirSuffix) throws IOException
{
final String outIndexFilePath;
- if (storageDirSuffix.endsWith("index.zip") ||
storageDirSuffix.endsWith("index.lz4")) {
+ if (storageDirSuffix.endsWith("index.zip") ||
storageDirSuffix.endsWith("index.lz4")
+ || (compressionFormat == CompressionUtils.Format.NONE &&
storageDirSuffix.endsWith("index"))) {
Review Comment:
[P2] Do not infer complete paths from an arbitrary suffix
**Finding:** With compressionFormat=none, any storageDirSuffix whose final
characters are "index" is treated as an already-complete segment path.
pushToPath normally receives an arbitrary directory suffix and must append the
partition name, so a suffix such as "index" can be written to the wrong
location; this branch also skips the colon-to-underscore normalization used for
HDFS paths.
**Suggestion:** Recognize only explicit complete segment path shapes, or
carry an explicit indication that the supplied suffix already names the segment
directory before bypassing partition suffixing and path normalization.
##########
extensions-core/hdfs-storage/src/main/java/org/apache/druid/storage/hdfs/HdfsDataSegmentPusher.java:
##########
@@ -227,6 +264,81 @@ private DataSegment pushToFilePath(File inDir, DataSegment
segment, String outIn
return dataSegment;
}
+ private DataSegment pushToFilePathNone(File inDir, DataSegment segment,
String outIndexDirPath) throws IOException
+ {
+ log.debug(
+ "Copying segment[%s] files to HDFS at location[%s]",
+ segment.getId(),
+ outIndexDirPath
+ );
+ final Path tmpIndexDir = new Path(StringUtils.format(
+ "%s/%s/%s/%s_index",
+ fullyQualifiedStorageDirectory.get(),
+ segment.getDataSource(),
+ UUIDUtils.generateUuid(),
+ segment.getShardSpec().getPartitionNum()
+ ));
+ final FileSystem fs = tmpIndexDir.getFileSystem(hadoopConfig);
+
+ fs.mkdirs(tmpIndexDir);
+ log.debug("Copying flat segment files from[%s] to [%s]", inDir,
tmpIndexDir);
+
+ final File[] files = inDir.listFiles();
Review Comment:
[P2] Clean up the temp directory on early failure
**Finding:** pushToFilePathNone creates tmpIndexDir before calling
inDir.listFiles(), but the cleanup finally block starts afterward. If listing
the local segment directory returns null or fails, the method throws before
cleanup; retries then leave orphaned HDFS temporary directories.
**Suggestion:** Move input-directory listing and temporary-directory setup
inside the try/finally scope, or explicitly delete the allocated temporary
parent before rethrowing every early failure.
--
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]
