xiangfu0 commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4229539647
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -76,6 +77,23 @@ public static void setDefaultTargetDocsPerChunk(int
defaultTargetDocsPerChunk) {
_defaultTargetDocsPerChunk = defaultTargetDocsPerChunk;
}
+ /// The chunk compression type for a raw column whose table config does not
name one. Defaults to LZ4.
+ ///
+ /// This is the raw value: the exemption that keeps metrics uncompressed is
applied by the caller, in
+ /// [org.apache.pinot.segment.local.segment.index.forward.ForwardIndexType],
so a caller resolving a
+ /// default for a specific column must go through that rather than reading
this directly.
+ ///
+ /// `volatile` because it is written by the starter thread and read later by
segment-build threads: the
+ /// thread-start happens-before covers today's call order, but a stale read
here would silently change
+ /// what gets written to disk.
+ public static ChunkCompressionType getDefaultCompressionType() {
+ return _defaultCompressionType;
+ }
+
+ public static void setDefaultCompressionType(ChunkCompressionType
defaultCompressionType) {
Review Comment:
**[severity: medium] The public setter accepts values the feature's own
guard exists to keep out.**
`setDefaultCompressionType` is public API in `pinot-segment-spi` and accepts
`null`, `DELTA`, `DELTADELTA` and `LZ4_LENGTH_PREFIXED`, while the
`isApplicableToRawIndex()` validation lives only in `ServiceStartableUtils`
(pinot-common) — a module embedders and batch-ingestion plugins don't go
through. The PR description itself calls the guard load-bearing: a `DELTA`
default makes every raw var-width column fail at build time (`DeltaCompressor`
throws on chunks whose size is not a multiple of 4), and `null` NPEs in
`ChunkCompressorFactory.getCompressor`.
Consider moving the validate-and-map step into `ForwardIndexConfig` (e.g. a
`setDefaultCompressionCodec(CompressionCodec)` that checks
`isApplicableToRawIndex()`), leaving `ServiceStartableUtils` to only parse the
string. That also removes the cross-module split where the class owning the
default doesn't own its invariant.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/openstruct/OpenStructColumnSplitter.java:
##########
@@ -567,7 +567,8 @@ private void writeDenseKeyColumn(String key)
// Reconcile dictionary + forward encoding with the final decision
(mirrors BaseSegmentCreator.adaptConfig);
// ForwardIndexCreatorFactory selects dict-vs-raw from the forward
config's EncodingType. A compression codec
- // applies only to the raw forward format (LZ4 preserves the dense child's
current on-disk layout); attaching
+ // applies only to the raw forward format (LZ4 preserves the dense child's
current on-disk layout, and is
+ // pinned here regardless of the cluster-wide default compression codec);
attaching
Review Comment:
**[severity: medium] OPEN_STRUCT keys now get different codecs depending on
how the segment was produced.**
This comment pins LZ4 on the build path, but the reload path is not pinned:
materialized keys with no explicit `indexes.forward` node resolve through
`FieldIndexConfigsUtil.forwardConfig` -> `ForwardIndexConfig.getDefault(RAW)`
(null codec), so a DICT->RAW conversion on reload
(`ForwardIndexHandler.rewriteDictToRawForwardIndex` ->
`ForwardIndexCreatorFactory`) writes the *configured* cluster default. With
`pinot.forward.index.default.compression.codec=ZSTANDARD` and an OPEN_STRUCT
table switching `defaultValueFieldConfig` to RAW, existing segments convert
keys to ZSTANDARD on reload while freshly built/committed segments write LZ4 —
the same key ends up with different codecs per segment, and the "keys are
always LZ4 raw" statements in `OpenStructIndexType`/`OpenStructIndexConfig`
docs become false.
Nothing breaks on read, but if LZ4 is meant to be pinned for these synthetic
columns it should be pinned on both paths (or the splitter should follow the
default on both).
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,66 @@ private static void
initForwardIndexConfig(PinotConfiguration instanceConfig) {
LOGGER.info("Setting forward index default target docs per chunk to:
{}", defaultTargetDocsPerChunk);
ForwardIndexConfig.setDefaultTargetDocsPerChunk(Integer.parseInt(defaultTargetDocsPerChunk));
}
+ String defaultCompressionCodec =
+
instanceConfig.getProperty(CommonConstants.ForwardIndexConfigs.CONFIG_OF_DEFAULT_COMPRESSION_CODEC);
+ if (defaultCompressionCodec != null) {
+ setDefaultCompressionCodec(defaultCompressionCodec);
+ }
+ }
+
+ /// Applies the cluster-wide default compression codec for raw forward
indexes.
+ ///
+ /// The value is a [CompressionCodec], the same spelling used by
`compressionCodec` in a table config,
+ /// so that an operator setting a cluster default and a table author setting
a column override write the
+ /// same word. Codecs that are not applicable to a raw forward index -- the
CLP family, `MV_ENTRY_DICT`,
+ /// `DELTA` -- are rejected rather than applied, since they describe
whole-index or dictionary formats
+ /// that cannot stand in for a chunk codec.
+ ///
+ /// Unlike the numeric defaults above, an unusable value is logged and
ignored instead of failing
+ /// startup: this one is routinely set as a *cluster* config, where throwing
would take down every
+ /// component that restarts after the bad value is saved. The warning names
the codecs that are
+ /// accepted, so a rejected value is self-correcting from the log.
+ ///
+ /// Scope, which this being a *cluster* config does not make as wide as it
sounds:
+ ///
+ /// - It is read once per JVM at startup, so after changing it a rolling
restart leaves restarted
+ /// instances writing the new codec while the rest keep the old one until
they restart too. Each
+ /// segment records the codec it was written with, so this mixes formats
without breaking reads.
+ /// - It reaches only components that call [#applyClusterConfig]: the
controller, broker, server and
+ /// minion. Segments built outside the cluster -- standalone, Spark or
Hadoop batch ingestion, and
+ /// `pinot-admin CreateSegment` -- keep the compiled-in default, and
because a reload ignores
+ /// defaults, nothing later reconciles the two.
Review Comment:
**[severity: low] "nothing later reconciles the two" is not quite true —
some reloads do apply the current default.**
`ForwardIndexHandler` ignores the default only when *deciding whether* to
rewrite. Any reload that rebuilds a raw forward index for another reason
applies the reloading server's current default via
`ForwardIndexCreatorFactory`: DICT->RAW conversion, codecSpec removal,
re-enabling a disabled forward index, and new derived/default columns
(`BaseDefaultColumnHandler`). So Spark-built LZ4 segments *are* partially
reconciled when such an operation runs — and during a rolling restart, two
replicas performing the same rewrite can produce locally differing codecs for
the same segment. Worth tightening this sentence (the wording in
`ForwardIndexType`'s doc, "when deciding whether a reload must rewrite", is the
accurate one).
--
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]
