xiangfu0 commented on code in PR #19898:
URL: https://github.com/apache/pinot/pull/19898#discussion_r4318652764
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/TarCompressionUtils.java:
##########
@@ -123,8 +126,10 @@ public static void createCompressedTarFile(File[]
inputFiles, File outputFile, S
throws IOException {
try (OutputStream fileOut = Files.newOutputStream(outputFile.toPath());
BufferedOutputStream bufferedOut = new BufferedOutputStream(fileOut);
- OutputStream compressorOut =
COMPRESSOR_STREAM_FACTORY.createCompressorOutputStream(compressorName,
- bufferedOut); TarArchiveOutputStream tarOut = new
TarArchiveOutputStream(compressorOut)) {
+ OutputStream compressorOut =
CompressorStreamFactory.LZ4_FRAMED.equalsIgnoreCase(compressorName)
Review Comment:
Split the global LZ4 writer/buffering change into #19946 and duplicate-name
validation into #19947. Both are independent PRs against master, with their own
release notes. Neither behavior remains in this PR; the benchmark description
now identifies the pre-split snapshot and links the LZ4 measurements to #19946.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/SegmentPushUtils.java:
##########
@@ -462,15 +461,41 @@ private static File
getSegmentMetadataFile(SegmentGenerationJobSpec spec, PinotF
return generateSegmentMetadataFile(fileSystem, URI.create(tarFilePath));
}
- /// Segments stored in the deep store do not have the .tar.gz extension.
- private static String getSegmentName(String tarFilePath) {
+ /// Resolves and validates the segment name from a supported gzip, LZ4, or
Zstandard archive path.
+ public static String getSegmentNameFromTarFile(String tarFilePath) {
String fileName = new File(tarFilePath).getName();
- String segmentName = fileName.endsWith(Constants.TAR_GZ_FILE_EXT)
- ? fileName.substring(0, fileName.length() -
Constants.TAR_GZ_FILE_EXT.length()) : fileName;
+ Preconditions.checkArgument(Constants.isSegmentTarFile(fileName),
"Unsupported segment archive: %s", fileName);
+ String extension =
Constants.SEGMENT_TAR_FILE_EXTENSIONS.values().stream().filter(fileName::endsWith)
+ .findFirst().orElseThrow();
+ String segmentName = fileName.substring(0, fileName.length() -
extension.length());
SegmentNameUtils.validatePartialOrFullSegmentName(segmentName);
return segmentName;
}
+ /// Rejects ambiguous uploads before HTTP or segment replacement starts.
Changing archive codecs in a reused output
+ /// directory can otherwise upload an old and a new archive of the same
segment in an arbitrary order.
+ /// Extensionless deep-store segment paths remain supported for metadata
push.
+ public static void validateUniqueSegmentNames(Collection<String>
segmentPaths) {
Review Comment:
Duplicate-name rejection now lives in #19947, with the `backward-incompat`
and `release-notes` labels and a compatibility notice at the top of its
description. That note covers recursive duplicate archives and using a clean
output directory or `pushFileNamePattern` to select one cohort. This PR no
longer introduces duplicate rejection.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/SegmentPushUtilsTest.java:
##########
@@ -503,8 +605,8 @@ public void testGetSegmentUriToTarPathMap()
String[] segmentFiles = new String[]{
outputDirURI.resolve("segment.tar.gz").toString(),
outputDirURI.resolve("stats_202201.tar.gz").toString(),
- outputDirURI.resolve("/2022/segment.tar.gz").toString(),
- outputDirURI.resolve("/2022/stats_202201.tar.gz").toString()
+ outputDirURI.resolve("/2022/segment_2022.tar.gz").toString(),
Review Comment:
Kept the original root/2022 duplicate archive layout in #19947 and now
assert that unfiltered discovery throws with both conflicting paths. The
2022-only pattern still succeeds. This PR restores the original discovery
fixture because the validation was split out.
##########
README.md:
##########
@@ -58,6 +58,19 @@ For getting started guides, deployment recipes, tutorials,
and more, please visi
<img src="https://pinot.apache.org/static/images/hero_diagram.svg";
align="center" alt="Apache Pinot"/>
+Batch ingestion jobs can opt into Zstandard segment archives in standalone,
Spark, and Hadoop runners:
Review Comment:
Moved the property and usage example to the batch job-spec reference in
https://github.com/pinot-contrib/pinot-docs/pull/1119. The root README addition
is removed. Upgrade guidance stays in the PR description, including the gzip
default and the downstream-consumer prerequisite for opting into Zstd.
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/test/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunnerTest.java:
##########
@@ -227,6 +356,34 @@ public boolean accept(File dir, String name) {
}
}
+ /// Exercises the same sibling-writing unpacker as the Parquet and ORC
readers without another plugin dependency.
+ public static class UnpackingCSVRecordReader extends CSVRecordReader {
+ @Override
+ public void init(File dataFile, @Nullable Set<String> fieldsToRead,
@Nullable RecordReaderConfig config)
+ throws IOException {
+ super.init(RecordReaderUtils.unpackIfRequired(dataFile, "csv"),
fieldsToRead, config);
+ }
+ }
+
+ /// Records copies to verify that plain local inputs are borrowed.
+ public static class TrackingLocalPinotFS extends LocalPinotFS {
+ private static TrackingLocalPinotFS _instance;
Review Comment:
Removed the static tracking instance and the test's global file-registry
registration/reset. Each ownership case injects its own tracking filesystem
into its runner, retaining the no-copy and source-preservation assertions for
success and failure. Focused tests and precommit checks pass. Fixed in
https://github.com/apache/pinot/commit/d13bd1276de6208afb26b546ce98c733ff946915.
--
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]
![https://pinot.apache.org/static/images/hero_diagram.svg"](https://pinot.apache.org/static/images/hero_diagram.svg"){kind=link}