xiangfu0 commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4227239837
##########
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: {}",
+ 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;
+ }
+ ChunkCompressionType compressionType;
+ try {
+ compressionType = ChunkCompressionType.valueOf(codec.name());
Review Comment:
**[dead code / duplicated mapping] This try/catch is unreachable, and it is
the third copy of the CompressionCodec-to-ChunkCompressionType mapping.**
After `isApplicableToRawIndex()` the codec is one of
PASS_THROUGH/SNAPPY/ZSTANDARD/LZ4/GZIP, all of which have same-named
`ChunkCompressionType` constants — so the catch can never fire and can never be
covered by a test. The canonical mapping already lives in
`ForwardIndexConfig`'s constructor switch (and `BaseSingleTreeBuilder:587` does
the same by-name conversion unguarded). The canonical map already has a
non-identity case (CLP* -> PASS_THROUGH), so if a future raw-applicable codec
is spelled differently, the constructor switch gets updated while this
warn-and-ignore path silently rejects the cluster default. Prefer the bare
`valueOf` (letting drift fail loudly) or one shared conversion helper used by
both sites.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -46,6 +46,7 @@ public class ForwardIndexConfig extends IndexConfig {
private static String _defaultTargetMaxChunkSize = "1MB";
private static int _defaultTargetMaxChunkSizeBytes = 1024 * 1024;
private static int _defaultTargetDocsPerChunk = 1000;
+ private static ChunkCompressionType _defaultCompressionType =
ChunkCompressionType.LZ4;
Review Comment:
**[latent visibility] Non-volatile mutable static written by the starter
thread and read by segment-build/reload threads.**
Publication currently relies only on the thread-start happens-before
(starters call `applyClusterConfig` before spawning Helix/segment threads),
which matches the four pre-existing sibling fields — but a stale read here
silently changes segment output rather than a chunk-size tweak. `volatile`
closes it for free:
```suggestion
private static volatile ChunkCompressionType _defaultCompressionType =
ChunkCompressionType.LZ4;
```
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexDefaultCompressionTypeTest.java:
##########
@@ -0,0 +1,67 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.index.forward;
+
+import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
+import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
+import org.apache.pinot.spi.data.FieldSpec.FieldType;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Tests that the configurable default compression type is honoured for raw
columns whose table config
+/// does not name a codec, and that metrics keep their uncompressed default
regardless.
+public class ForwardIndexDefaultCompressionTypeTest {
+
+ @AfterMethod(alwaysRun = true)
+ public void tearDown() {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.LZ4);
+ }
+
+ @Test
+ public void testDefaultIsLZ4ForNonMetric() {
Review Comment:
**[test robustness] `testDefaultIsLZ4ForNonMetric` asserts the JVM-global
initial value but the class has no `@BeforeMethod` reset (only
`@AfterMethod`).**
It passes in Maven only because the root surefire runs `reuseForks=false`;
in a shared-JVM run (IDE "run all in package", a future surefire change, or any
earlier test that sets the static without an `alwaysRun` cleanup — e.g. its own
tearDown skipped by a config-method failure) this test fails order-dependently
with `ZSTANDARD != LZ4` and cannot be reproduced in isolation.
`ServiceStartableUtilsTest` in this same PR already uses the `@BeforeMethod` +
`@AfterMethod` pattern — mirroring it here is one line.
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/ServiceStartableUtilsTest.java:
##########
@@ -0,0 +1,110 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
+import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Covers parsing of the `pinot.forward.index.default.compression.codec`
cluster config.
+///
+/// The value names a
[org.apache.pinot.spi.config.table.FieldConfig.CompressionCodec], the same
+/// vocabulary a table config uses, and anything that cannot serve as a raw
forward index chunk codec is
+/// ignored rather than applied: it would otherwise be injected after
table-config validation has run and
+/// fail at segment build time, across every table at once.
+public class ServiceStartableUtilsTest {
+
+ @BeforeMethod
+ public void setUp() {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.LZ4);
+ }
+
+ @AfterMethod(alwaysRun = true)
+ public void tearDown() {
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.LZ4);
+ }
+
+ @DataProvider
+ public Object[][] applicableCodecs() {
+ return new Object[][]{
+ {"ZSTANDARD", ChunkCompressionType.ZSTANDARD},
+ {"zstandard", ChunkCompressionType.ZSTANDARD},
+ {" ZSTANDARD ", ChunkCompressionType.ZSTANDARD},
+ {"SNAPPY", ChunkCompressionType.SNAPPY},
+ {"GZIP", ChunkCompressionType.GZIP},
+ {"PASS_THROUGH", ChunkCompressionType.PASS_THROUGH},
+ {"LZ4", ChunkCompressionType.LZ4}
Review Comment:
**[vacuous test row] The `{"LZ4", ChunkCompressionType.LZ4}` row cannot
detect a no-op.**
`setUp()` already sets the default to LZ4 before every method, so this row
passes even if `setDefaultCompressionCodec("LZ4")` silently does nothing (e.g.
a future refactor that ignores LZ4). Seeding a non-LZ4 value before the call —
as `testAnIgnoredValueDoesNotUndoAnEarlierValidOne` does — would make the row
meaningful.
##########
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:
**[doc gap] The value is applied once per instance at startup, so a rolling
restart leaves instances with different defaults.**
After changing the ZK cluster config, restarted servers/minions write the
new codec while not-yet-restarted ones keep the old one, so realtime commits
and minion rewrites for the same table mix codecs during the rollout window
(and a server crash-restarting weeks later silently picks up the new default
where its peers did not). No correctness break — each segment records its
actual codec — but these per-instance-startup semantics are not documented for
this key, and operators generally expect cluster configs to apply cluster-wide
at once. Worth a sentence in the method doc and on
`CONFIG_OF_DEFAULT_COMPRESSION_CODEC`.
##########
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:
**[consistency] New lenient parse (trim + uppercase) diverges from the
strict enum parse used for the other enum-valued cluster config in this same
file.**
`initFieldSpecConfig` parses `MaxLengthExceedStrategy` with a bare
case-sensitive `valueOf` while this key accepts `"snappy"` / `" SNAPPY "`.
Two enum-valued cluster configs in one class now follow different spelling
rules, so an operator who learns the lenient spelling from one gets a silent
ignore (or a crash) from the other. Either keep strict `valueOf` here too, or
extract one lenient enum-parse helper and use it for both.
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/ServiceStartableUtilsTest.java:
##########
@@ -0,0 +1,110 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import org.apache.pinot.segment.spi.compression.ChunkCompressionType;
+import org.apache.pinot.segment.spi.index.ForwardIndexConfig;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Covers parsing of the `pinot.forward.index.default.compression.codec`
cluster config.
+///
+/// The value names a
[org.apache.pinot.spi.config.table.FieldConfig.CompressionCodec], the same
+/// vocabulary a table config uses, and anything that cannot serve as a raw
forward index chunk codec is
+/// ignored rather than applied: it would otherwise be injected after
table-config validation has run and
+/// fail at segment build time, across every table at once.
+public class ServiceStartableUtilsTest {
Review Comment:
**[convention] CLAUDE.md: "Add class-level Javadoc for new classes; describe
behavior and thread-safety."**
Both new test classes (this one and
`ForwardIndexDefaultCompressionTypeTest`) have class docs describing behavior
but say nothing about thread-safety — which is material here, because every
test method mutates the process-global `ForwardIndexConfig` static, making
these classes unsafe to run in parallel with each other or with anything that
builds segments. A one-line note ("mutates JVM-global ForwardIndexConfig
defaults; must not run in parallel with other users of that static") is exactly
what would warn whoever later enables parallel TestNG execution.
--
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]
