gortiz commented on code in PR #19898:
URL: https://github.com/apache/pinot/pull/19898#discussion_r4359272133
##########
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:
Before this change, the Hadoop and Spark TAR and URI push runners ignored
`pushFileNamePattern`. Now they apply it, so a spec that already had a pattern
set (for example one copied from a metadata push job) pushes a subset of the
segments, or nothing. I think the new behavior is right, but it would be good
to mention it in the PR description so it lands in the release notes.
##########
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:
`LocalPinotFS.listFiles` returns plain paths, so this branch changes the
generated URI for local outputs. Before, `URI.create(path)` decoded `%XX`, so a
URL-encoded archive name like `a%3Ab.tar.gz` became `file:/out/a:b.tar.gz`. Now
`File.toURI()` keeps the `%` literal and the URI is `file:/out/a%253Ab.tar.gz`.
The same applies with `segmentUriPrefix`, through `getRawPath()`.
The new URI points to the real file, so I think it is more correct. But it
is a behavior change for local URI and metadata push, and no test covers it:
`testSegmentArchiveScanning` passes `file:` URI strings, which skip this
branch. A test with a plain absolute path and an encoded name, with and without
a prefix, would pin it down.
##########
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:
This early return, and the same one in the Hadoop and Spark base runners,
ends the job without any log when nothing matches. Now that the push pattern
applies to more runners and a codec switch can leave archives with an
unexpected extension, an empty selection is easier to hit. A WARN with the
output dir and the push pattern would make that case visible.
`BaseSegmentPushJobRunner` (standalone) does not have this guard. With
consistent push enabled it still calls `ConsistentDataPushUtils.preUpload` with
an empty `segmentsTo`, which replaces all existing segments with nothing. That
predates this PR, but it would be nice if both paths behaved the same way.
--
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]
