xiangfu0 commented on code in PR #19898:
URL: https://github.com/apache/pinot/pull/19898#discussion_r4289316439
##########
pinot-spi/src/main/java/org/apache/pinot/spi/ingestion/batch/spec/SegmentGenerationJobSpec.java:
##########
@@ -228,6 +232,17 @@ public void setSegmentCreationJobParallelism(int
segmentCreationJobParallelism)
_segmentCreationJobParallelism = segmentCreationJobParallelism;
}
+ public String getSegmentTarCompressionCodec() {
+ return _segmentTarCompressionCodec == null ? "gzip" :
_segmentTarCompressionCodec;
+ }
+
+ /// Selects the archive codec for standalone, Spark, and Hadoop segment
generation jobs.
+ public void setSegmentTarCompressionCodec(String segmentTarCompressionCodec)
{
+ Preconditions.checkArgument("gzip".equals(segmentTarCompressionCodec) ||
"zstd".equals(segmentTarCompressionCodec),
Review Comment:
Added a comment at the setter: generation accepts gzip/zstd, while archive
discovery also consumes existing LZ4 archives. The generation codec set is
unchanged.
Follow-up:
[38ef4bb6ea](https://github.com/apache/pinot/commit/38ef4bb6eaf4b97c645768b7fe8dacf00f8ce734).
##########
pinot-spi/src/main/java/org/apache/pinot/spi/ingestion/batch/spec/SegmentGenerationJobSpec.java:
##########
@@ -58,6 +59,9 @@ public class SegmentGenerationJobSpec implements Serializable
{
/// Segment creation job parallelism.
private int _segmentCreationJobParallelism;
+ /// Codec for segment archives: `gzip` (default) or `zstd`. Metadata
sidecars always use gzip.
+ private String _segmentTarCompressionCodec = "gzip";
Review Comment:
The missing explicit UID predates this PR. An arbitrary UID would not match
historical computed UIDs; establishing compatibility needs historical fixtures
and a nested-spec audit. I left that policy unchanged here and am not claiming
mixed-version Spark compatibility.
##########
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:
Added an upgrade note covering duplicate logical segment names, including
archives in different recursively listed subdirectories or with different
codecs. It explains that pushes now fail fast and recommends removing stale
archives or narrowing the push file pattern.
Follow-up:
[38ef4bb6ea](https://github.com/apache/pinot/commit/38ef4bb6eaf4b97c645768b7fe8dacf00f8ce734).
--
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]
