xiangfu0 commented on code in PR #19898:
URL: https://github.com/apache/pinot/pull/19898#discussion_r4363167452
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-spark-3/src/main/java/org/apache/pinot/plugin/ingestion/batch/spark3/SparkSegmentMetadataPushJobRunner.java:
##########
@@ -107,6 +103,9 @@ public void run()
// Collect files to process
List<String> segmentsToPush = getSegmentsToPush(outputDirFS, outputDirURI);
+ if (segmentsToPush.isEmpty()) {
Review Comment:
Fixed in
https://github.com/apache/pinot/commit/ea29c2f4ae3b3277fa867e5538e4d3f2cc487ebc:
shared archive selection now warns with the output directory and push pattern
when empty, covering all runner variants. Standalone also returns before
replacement preparation/preUpload and upload; a regression enables consistent
push and filters out every archive. All 53 focused JDK 25 tests,
affected-module checks, and independent review passed.
##########
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:
Added an explicit Compatibility/release note to the PR description: Hadoop
and Spark TAR/URI push now honor pushFileNamePattern, which they previously
ignored. It calls out that existing specs may select fewer segments or none and
advises reviewing or removing the pattern when upgrading.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/SegmentPushUtils.java:
##########
@@ -684,12 +693,13 @@ 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()
+ ? new File(file).toURI() : URI.create(file);
Review Comment:
Covered in
https://github.com/apache/pinot/commit/ea29c2f4ae3b3277fa867e5538e4d3f2cc487ebc
by extending the existing archive-scanning test across gzip/LZ4/Zstd. It
creates a real a%3Ab archive, supplies the plain absolute path, and verifies
the literal percent is preserved as %253A in the generated URI both with and
without segmentUriPrefix, while the map value retains the original file path.
--
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]
