xiangfu0 commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4229542356
##########
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.
+ /// - It is a process-global default, so in a single-JVM multi-role
deployment the last role to start
+ /// wins for the whole process.
+ ///
+ /// These all hold for the three forward index defaults above as well; they
are spelled out here
+ /// because this is the first of them an operator is routinely expected to
set.
+ @VisibleForTesting
+ static void setDefaultCompressionCodec(String defaultCompressionCodec) {
+ CompressionCodec codec;
+ try {
+ codec =
CompressionCodec.valueOf(defaultCompressionCodec.trim().toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ LOGGER.warn("Unknown forward index default compression codec: {},
expected one of: {}, keeping: {}",
+ defaultCompressionCodec, rawIndexCompressionCodecs(),
ForwardIndexConfig.getDefaultCompressionType());
+ return;
+ }
+ if (!codec.isApplicableToRawIndex()) {
+ LOGGER.warn("Forward index default compression codec: {} is not
applicable to raw forward indexes, expected "
+ + "one of: {}, keeping: {}", codec, rawIndexCompressionCodecs(),
+ ForwardIndexConfig.getDefaultCompressionType());
+ return;
+ }
+ // Every raw-applicable codec has a same-named ChunkCompressionType; if
that ever stops holding, this
+ // should fail loudly rather than silently keep the old default.
+ ChunkCompressionType compressionType =
ChunkCompressionType.valueOf(codec.name());
Review Comment:
**[severity: low] This is now the 4th copy of the `CompressionCodec` ->
`ChunkCompressionType` mapping, and the only load-bearing one that is
name-based.**
The canonical mapping is the explicit switch in `ForwardIndexConfig`'s
`@JsonCreator` (where CLP* map to PASS_THROUGH and MV_ENTRY_DICT to null — i.e.
it is deliberately *not* name-based); `BaseSingleTreeBuilder` and
`CodecPipelineIntegrationTest` carry two more. A by-name `valueOf` still
compiles when either enum changes and only fails at runtime — at startup, in
this case. Extracting the existing switch into a static `ForwardIndexConfig`
helper and calling it here (and from the constructor) keeps one authoritative
mapping and gives this call the same fail-loudly behavior the comment asks for.
##########
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
Review Comment:
**[severity: low] Doc inaccuracy: `DELTA` is not a "whole-index or
dictionary format".**
`DELTA`/`DELTADELTA` are chunk codecs (`ChunkCompressionType.DELTA(6)`,
`DELTADELTA(7)`); they are rejected here because table-config validation
rejects them everywhere and their compressors throw on chunks whose size is not
a multiple of 4 — which the test class doc states correctly. Also `DELTADELTA`
is missing from the enumerated list. Suggest splitting the sentence: CLP family
/ `MV_ENTRY_DICT` are whole-index or dictionary formats; `DELTA`/`DELTADELTA`
are chunk codecs that are not valid for general raw columns.
##########
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
Review Comment:
**[severity: low] The "thread-start happens-before covers today's call
order" claim doesn't hold for all launch paths.**
`StartServiceManagerCommand.startBootstrapServicesInParallel` runs
BROKER/SERVER/MINION `init()` on sibling threads concurrently,
`QuickstartRunner.startAll` starts the server before the minion's `init()`, and
the PinotServiceManager REST endpoint adds roles to an already-running JVM — in
those paths a later role's `applyClusterConfig` runs after segment-build
threads exist, with no thread-start edge. So the `volatile` is required today
rather than defensive, "last role to start wins" is nondeterministic under
parallel bootstrap, and the closing claim in `ServiceStartableUtils` that
"these all hold for the three forward index defaults above as well" overstates
things — those siblings are non-volatile (and `setDefaultTargetMaxChunkSize`
writes its string/bytes pair as two separate stores). Suggest softening the
sentence; the `volatile` itself is the right call.
--
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]
