arunkumarucet commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4227370142
##########
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:
Documented rather than fixed, and I want to be explicit that it is a real
limitation.
The method doc now spells out all three scope caveats together: the value is
read once per JVM at startup, it reaches only components that call
`applyClusterConfig`, and it is a process-global default so in a single-JVM
multi-role deployment the last role to start wins for the whole process. It
also says these hold for the three sibling defaults, and that they are spelled
out here because this is the first of them an operator is routinely expected to
set.
Fixing the single-JVM case properly means making the default per-role rather
than per-process, which is a change to all four defaults and to
`ForwardIndexConfig`'s contract — happy to do it as a follow-up if you think it
is worth it, but it felt wrong to smuggle into this PR.
##########
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:
Agreed the doc over-promised. `ForwardIndexType.getDefaultCompressionType`'s
javadoc now says the config is applied at startup by components that read
cluster config, and that segments built outside the cluster — standalone, Spark
or Hadoop batch ingestion — keep LZ4. The same three caveats, including this
one, are listed on the parsing method in `ServiceStartableUtils`.
I left the behaviour alone: making `SegmentGenerationTaskRunner` and
`pinot-admin CreateSegment` honour a cluster config they never read would mean
giving them a ZK connection, which is a much larger change and would be odd for
the sibling defaults to not share.
##########
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:
Verified and documented. `noDictionaryConfig` is consumed by `keySet()` only
— `ForwardIndexType` L239-241 and `DictionaryIndexType` L127-128 — so the
per-column codec value has always been discarded, and those columns fall
through to the null-codec path in `ForwardIndexCreatorFactory`.
You are right that this PR changes the substitute from a deterministic LZ4
to a cluster-mutable value, so I have said so in the method javadoc: it now
notes that the legacy config's codec value is discarded, that only its key set
is read, and that such a column used to land on a hardcoded LZ4 and now follows
the configured default.
I did not make it honour the legacy value here. That would be a behaviour
fix to a config whose value has been ignored for a long time, and it belongs in
its own change where it can be considered on its merits rather than as a side
effect of adding a default.
##########
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:
Good catch on the rationale, which is the part that actually broke. The
comment said "LZ4 preserves the dense child's current on-disk layout", which
was only true while LZ4 *was* the default; it now reads that LZ4 is pinned
there regardless of the cluster-wide default compression codec.
I kept both sites pinned to LZ4 rather than routing them through the
default. The split re-encodes an existing dense child, so the codec choice
there is about matching what is already on disk, not about what new columns
should use — routing it through a mutable default would make the re-encode
depend on cluster config at the moment of the split. Happy to revisit if you
read that differently.
--
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]
