arunkumarucet commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4227373790
##########
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:
Verified — `applyClusterConfig` rewrites `pinot.all.X` to `pinot.<role>.X`
(L89-96), and `initForwardIndexConfig` reads the bare key, so the role-prefixed
spelling never matches and no warn path fires.
I have not changed it here, deliberately: reading the role-prefixed spelling
for this one key would make it the only one of the four that does, which trades
an invisible trap for an inconsistent one. It seems better fixed for all four
together — either by having `initForwardIndexConfig` check the role-prefixed
key as well, or by warning when a `pinot.<role>.forward.index.default.*` key is
present but unread. Happy to do that as a follow-up, or in this PR if you would
rather it not ship with the gap.
##########
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:
The contradiction is fair, and I would rather resolve it by making the
siblings lenient than by making this one throw.
A cluster config that throws on a typo takes down every component that
restarts after the value is saved, including ones that were healthy — that is a
worse failure than building LZ4 segments, and it is why I did not copy the
numeric defaults' behaviour. The precedent in this class is
`initFieldSpecConfig`, which warn-and-ignores.
On discoverability: the warning now names the accepted codecs, so a typo'd
value logs `Unknown forward index default compression codec: ZSTD, expected one
of: [PASS_THROUGH, SNAPPY, ZSTANDARD, LZ4, GZIP], keeping: LZ4`. That is
self-correcting from the log without needing to inspect segment metadata.
I did not alias `ZSTD`. The value deliberately names a `CompressionCodec`,
which is the vocabulary a table config uses, and `ZSTD` is not one — aliasing
it here would mean `compressionCodec: ZSTD` works as a cluster default but not
in a table config. If you would prefer the alias anyway I will add it, but it
seemed worth keeping one spelling per concept.
##########
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 =
Review Comment:
Documented. The method javadoc now states that the value 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 — and that this mixes formats without breaking reads, since each
segment records the codec it was written with.
##########
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));
Review Comment:
Fair inconsistency to point out. I kept the lenient parse here: this value
is typed by operators into a cluster config, where `snappy` is a very natural
thing to write, and the strict alternative silently ignores it.
That argues for making `initFieldSpecConfig` lenient too rather than making
this strict, but changing how an existing key parses is a behaviour change to
someone else's config and felt out of scope for this PR. If you would like one
shared lenient enum-parse helper used by both, say the word and I will add it
here.
--
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]
