xiangfu0 commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4227230370
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ 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);
Review Comment:
**[correctness] Multi-role single-JVM deployments: the last starter to run
silently overwrites the JVM-global default for all roles.**
`ForwardIndexConfig.setDefaultCompressionType` mutates a process-global
static, and every role starter (controller/broker/server/minion) re-runs
`initForwardIndexConfig` from its own merged config. In a single-JVM deployment
(quickstart, `ServiceManager`), `addConfigIfNotExists` gives each component's
local conf precedence over the ZK value, so if the roles' effective values
differ (e.g. server conf sets `ZSTANDARD`, minion conf omits it while ZK says
`LZ4`), the last-started role flips the shared static *after* earlier roles
have begun building segments. A segment build that straddles the flip can even
get mixed codecs across columns, since `ForwardIndexCreatorFactory` resolves
the default per column at creator-construction time. The three sibling defaults
share this mechanism, but this PR adds the first value operators are explicitly
invited to set per-instance.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -3024,6 +3024,8 @@ public static class ForwardIndexConfigs {
"pinot.forward.index.default.target.max.chunk.size";
public static final String CONFIG_OF_DEFAULT_TARGET_DOCS_PER_CHUNK =
"pinot.forward.index.default.target.docs.per.chunk";
+ public static final String CONFIG_OF_DEFAULT_COMPRESSION_CODEC =
+ "pinot.forward.index.default.compression.codec";
Review Comment:
**[scope gap] Segment builds outside the cluster never see this config —
tables end up permanently mixed-compression.**
Only the four in-cluster starters call
`ServiceStartableUtils.applyClusterConfig`. Standalone/Spark/Hadoop batch
ingestion (`SegmentGenerationTaskRunner`) and `pinot-admin CreateSegment` build
segments in their own JVM through the same `ForwardIndexCreatorFactory`
null-codec path, so they keep the compiled-in LZ4 while server/minion-built
segments of the same table use the configured codec. Because
`shouldRewriteRawForwardIndex` deliberately ignores defaults, reload never
reconciles them. The same gap exists for the sibling forward-index defaults,
but the new doc on `ForwardIndexType.getDefaultCompressionType` ("unless
`pinot.forward.index.default.compression.codec` says otherwise") implies
cluster-wide reach it doesn't have — worth a doc note here stating the config
only affects components that apply cluster config at startup.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ 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);
Review Comment:
**[operator trap] The documented `pinot.all.` spelling of this key is
silently dropped.**
`applyClusterConfig`'s javadoc advertises that `pinot.all.*` cluster keys
are rewritten to role-specific keys
(`pinot.server.forward.index.default.compression.codec`, ...), but
`initForwardIndexConfig` reads only the bare key
`pinot.forward.index.default.compression.codec`. An operator who follows the
documented convention and sets
`pinot.all.forward.index.default.compression.codec=ZSTANDARD` gets no warning
at all (the warn paths in `setDefaultCompressionCodec` never run because the
bare key is absent), and the cluster keeps building LZ4. Pre-existing for the
sibling keys, but the new key inherits the trap and it is invisible by design —
consider also checking the role-prefixed spelling or logging when it is present.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java:
##########
@@ -335,11 +335,20 @@ private ForwardIndexConfig
createConfigFromFieldConfig(FieldConfig fieldConfig,
return builder.build();
}
+ /// The compression type used for a raw column whose table config does not
name one.
+ ///
+ /// Metrics stay uncompressed: they are usually small fixed-width values
where compression costs more
+ /// than it saves. Everything else uses
[ForwardIndexConfig#getDefaultCompressionType()], which is LZ4
+ /// unless `pinot.forward.index.default.compression.codec` says otherwise.
+ ///
+ /// This is a default, not a table config value, so changing it does not
rewrite existing segments:
+ /// `ForwardIndexHandler` deliberately ignores it when deciding whether a
reload must rewrite a forward
+ /// index.
public static ChunkCompressionType
getDefaultCompressionType(FieldSpec.FieldType fieldType) {
if (fieldType == FieldSpec.FieldType.METRIC) {
return ChunkCompressionType.PASS_THROUGH;
} else {
- return ChunkCompressionType.LZ4;
+ return ForwardIndexConfig.getDefaultCompressionType();
Review Comment:
**[behavior change] Columns declared via legacy `noDictionaryConfig: {"col":
"SNAPPY"}` now follow the mutable cluster default instead of deterministic
LZ4.**
`noDictionaryConfig` values are consumed by `keySet()` only (this file
L239-241, `DictionaryIndexType` L127-128) — the per-column codec value is never
parsed into the `ForwardIndexConfig`, so such columns carry a null chunk
compression type and get filled at `ForwardIndexCreatorFactory:107`. Before
this PR that fill-in was the constant LZ4; now it is whatever the cluster
default is set to. So a user's *explicit* legacy `"SNAPPY"` is overridden by
the cluster knob, and `shouldRewriteRawForwardIndex` (null
`newCompressionType`) will never rewrite it back. The value-ignoring is
pre-existing, but this PR changes the substitute from deterministic to
cluster-mutable — either honor the legacy value or document the interaction.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java:
##########
@@ -335,11 +335,20 @@ private ForwardIndexConfig
createConfigFromFieldConfig(FieldConfig fieldConfig,
return builder.build();
}
+ /// The compression type used for a raw column whose table config does not
name one.
+ ///
+ /// Metrics stay uncompressed: they are usually small fixed-width values
where compression costs more
+ /// than it saves. Everything else uses
[ForwardIndexConfig#getDefaultCompressionType()], which is LZ4
Review Comment:
**[missed call sites] "Everything else uses the default" is not true for
OPEN_STRUCT-derived columns — `OpenStructColumnSplitter` still hardcodes LZ4.**
`OpenStructColumnSplitter` pins `CompressionCodec.LZ4` for materialized raw
key columns (~L575) and `ChunkCompressionType.LZ4` for the sparse remainder
column (~L767). With a cluster default of e.g. ZSTANDARD, the dense child is
originally written with ZSTANDARD (via this default), then the split re-encodes
it as LZ4 — the opposite of the adjacent rationale comment "LZ4 preserves the
dense child's current on-disk layout", which assumed LZ4 *was* the default. The
precondition message there ("always use a dictionary-encoded or LZ4 raw forward
index") also becomes inaccurate. Either route those sites through
`ForwardIndexType.getDefaultCompressionType(fieldType)` / leave the codec null,
or update the comments to say "LZ4 regardless of the cluster default".
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -134,6 +141,55 @@ 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.
+ @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: {}",
Review Comment:
**[silent misconfiguration + policy asymmetry] A near-miss spelling keeps
LZ4 forever with only a startup WARN, while the sibling configs three blocks
above crash on a typo.**
`ZSTD` — the spelling of the codecSpec DSL and of the zstd tool itself,
explicitly anticipated by this PR's own `unrecognisedValues` test — is
warn-and-ignored, so the operator sees every component start cleanly, believes
ZSTANDARD is active, and only discovers otherwise by inspecting segment
metadata later. There is no way to retroactively distinguish "default never
applied" from "tables overrode it". Meanwhile a typo in
`pinot.forward.index.default.target.docs.per.chunk` still throws from the
uncaught `Integer.parseInt` and takes down every restarting component — the
exact risk the doc comment above cites as justification for *not* throwing
here. One policy for all four defaults (uniformly lenient-with-warn, or
fail-fast plus controller-side validation), or at least aliasing `ZSTD`, would
remove both the trap and the contradiction.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -76,6 +77,16 @@ public static void setDefaultTargetDocsPerChunk(int
defaultTargetDocsPerChunk) {
_defaultTargetDocsPerChunk = defaultTargetDocsPerChunk;
}
+ /// The chunk compression type applied to a raw column whose table config
does not name one, except
Review Comment:
**[doc contract] This javadoc promises a metric exemption the method does
not implement.**
"...except for metrics, which stay uncompressed" is enforced only in
`ForwardIndexType.getDefaultCompressionType(FieldType)` in pinot-segment-local
— a module this SPI getter cannot reference. The getter itself returns the
configured codec for every field type. A future caller in segment-spi or below
segment-local that trusts this doc and calls the getter directly for a metric
column would start compressing metrics once the cluster default is set,
violating the invariant the PR's own `testConfiguredDefaultDoesNotAffectMetric`
pins down. Reword to state that the metric exemption is applied by the caller
(`ForwardIndexType`), not by this method.
--
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]
