xiangfu0 commented on code in PR #19947:
URL: https://github.com/apache/pinot/pull/19947#discussion_r4347066538
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-hadoop/src/main/java/org/apache/pinot/plugin/ingestion/batch/hadoop/HadoopSegmentTarPushJobRunner.java:
##########
@@ -80,17 +79,12 @@ public void run() {
throw new RuntimeException("Unable to list all files under outputDirURI
- '" + outputDirURI + "'");
}
- List<String> segmentsToPush = new ArrayList<>();
- for (String file : files) {
- if (file.endsWith(Constants.TAR_GZ_FILE_EXT)) {
- segmentsToPush.add(file);
- }
+ List<String> segmentsToPush = new ArrayList<>(
+ SegmentPushUtils.getSegmentUriToTarPathMap(outputDirURI,
_spec.getPushJobSpec(), files).values());
Review Comment:
Routing through `getSegmentUriToTarPathMap` means the Hadoop/Spark runners
now honor `pushJobSpec.pushFileNamePattern`, which they previously ignored
(they pushed every `.tar.gz`). This matches the standard runner and is the
right behavior, but it's a silent change for anyone who set that pattern on a
Hadoop/Spark job. Worth a line in the PR description / release notes.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/SegmentPushUtils.java:
##########
@@ -684,17 +702,20 @@ public static Map<String, String>
getSegmentUriToTarPathMap(URI outputDirURI, Pu
}
}
- URI uri = URI.create(file);
+ URI uri = outputDirURI != null &&
"file".equals(outputDirURI.getScheme()) && new File(file).isAbsolute()
Review Comment:
Nice guard for the null `outputDirURI` here. One thing it doesn't cover:
`generateSegmentTarURI` (called just below) still dereferences `dirURI` when
both prefix and suffix are empty, so a null `outputDirURI` with no
prefix/suffix NPEs. It's pre-existing and reachable from the METADATA path in
IngestionUtils, so not introduced here — but since this commit touches the
nullable-output handling, a null-guard there (and a test case without a prefix)
would close it out.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/SegmentPushUtils.java:
##########
@@ -462,6 +464,20 @@ private static File
getSegmentMetadataFile(SegmentGenerationJobSpec spec, PinotF
return generateSegmentMetadataFile(fileSystem, URI.create(tarFilePath));
}
+ /// Rejects ambiguous uploads before HTTP or segment replacement starts.
Archives in different output directories
+ /// can otherwise overwrite the same segment in an arbitrary order.
+ /// Extensionless deep-store segment paths remain supported for metadata
push.
+ public static void validateUniqueSegmentNames(Collection<String>
segmentPaths) {
+ Map<String, String> pathsBySegmentName = new HashMap<>();
+ for (String path : segmentPaths) {
+ String segmentName = getSegmentName(path);
+ String previous = pathsBySegmentName.putIfAbsent(segmentName, path);
+ Preconditions.checkArgument(previous == null,
+ "Multiple archives for segment %s: %s and %s. Remove stale archives
or narrow the push file pattern.",
Review Comment:
Minor: with 3+ archives colliding on one name, the message only names the
first-seen path and the current one, not the ones in between. Fine for
aborting, just slightly less helpful when debugging a wide fan-out.
--
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]
