arunkumarucet commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4244902061
##########
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:
Done. The switch is extracted into
`ForwardIndexConfig.toChunkCompressionType` and called from both the
`@JsonCreator` constructor and the new setter, so there is one authoritative
mapping and it keeps the non-identity cases (CLP family to `PASS_THROUGH`,
`MV_ENTRY_DICT` to null) rather than relying on names matching.
The helper is `private`: its only callers are in `ForwardIndexConfig`, and
it maps precisely the codecs the setter exists to reject, so exposing it would
offer a way around those guards.
##########
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:
Fixed. The sentence now distinguishes the two reasons: the CLP family and
`MV_ENTRY_DICT` name whole-index or dictionary-id formats rather than chunk
codecs; `DELTA` and `DELTADELTA` *are* chunk codecs, rejected because table
config validation rejects them on every column and their compressors throw on
chunks that are not a multiple of 4 bytes. `DELTADELTA` is now named alongside
`DELTA` everywhere the list appears.
##########
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:
You are right, and the claim is now gone. The field's javadoc says
`volatile` is needed because the thread-start happens-before does not cover
every launch path, naming `startBootstrapServicesInParallel` (sibling threads
per role) and the PinotServiceManager REST endpoint adding a role to a JVM that
is already serving.
The "these all hold for the siblings too" sentence is also corrected: the
first two caveats hold for them, while the third is *worse* for the siblings,
since under a parallel bootstrap the roles race and their statics are not
`volatile`.
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/ServiceStartableUtilsTest.java:
##########
@@ -0,0 +1,120 @@
+/**
+ * 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.
+///
+/// Not thread-safe: every method mutates the process-global
[ForwardIndexConfig] default, so this class
+/// must not run in parallel with other tests, or with anything that builds
segments.
+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}
+ };
+ }
+
+ @Test(dataProvider = "applicableCodecs")
+ public void testApplicableCodecIsApplied(String configured,
ChunkCompressionType expected) {
+ // Seed something other than the value under test, so a row whose
expectation happens to equal the
+ // starting default -- LZ4 -- still fails if the codec is never applied.
+ ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.GZIP);
+ if (expected == ChunkCompressionType.GZIP) {
+
ForwardIndexConfig.setDefaultCompressionType(ChunkCompressionType.SNAPPY);
+ }
+
+ ServiceStartableUtils.setDefaultCompressionCodec(configured);
+
+ assertEquals(ForwardIndexConfig.getDefaultCompressionType(), expected);
+ }
+
+ /// `DELTA` and `DELTADELTA` matter most here: they are rejected by
table-config validation on every
+ /// column, and their compressors throw on any chunk whose size is not a
multiple of 4 bytes, so every
+ /// raw var-width column in the cluster would fail to build if one slipped
through as a default.
+ @DataProvider
+ public Object[][] codecsNotApplicableToRawIndexes() {
+ return new Object[][]{
+ {"DELTA"}, {"DELTADELTA"}, {"MV_ENTRY_DICT"}, {"CLP"}, {"CLPV2"},
{"CLPV2_ZSTD"}, {"CLPV2_LZ4"}
+ };
+ }
+
+ @Test(dataProvider = "codecsNotApplicableToRawIndexes")
Review Comment:
Fixed, and more broadly than this one test. The class now seeds a SNAPPY
baseline in `@BeforeMethod`, so no assertion in it expects its own starting
value — the rejection tests and `testUnrecognisedValueKeepsCurrentDefault`
included. A regression that wrongly applied LZ4, or mapped a rejected codec to
LZ4, now fails.
##########
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);
Review Comment:
Added, in both directions.
Consumer side: `testTheConfigKeyReachesTheDefault` builds a
`PinotConfiguration` holding `CONFIG_OF_DEFAULT_COMPRESSION_CODEC` and drives
`initForwardIndexConfig`, with `testAbsentConfigKeyLeavesTheDefaultAlone` for
the other branch. `initForwardIndexConfig` is `@VisibleForTesting` for this.
Producer side, which was the larger hole: nothing proved the default
actually reached a creator. `ForwardIndexCreatorFactoryTest` now has
`testCreatorFollowsTheConfiguredDefaultCompressionType` and
`testMetricColumnIgnoresTheConfiguredDefaultCompressionType`, which build a
creator from a codec-less config and assert
`getRawForwardIndexChunkCompressionType()`. A refactor that inlined LZ4 in the
factory would now fail.
I also restored coverage of the compiled-in value itself:
`testCompiledInDefaultIsLZ4` captures the default in `@BeforeClass`, before any
`@BeforeMethod` sets one, which is what every "segments built outside the
cluster keep LZ4" sentence rests on.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexDefaultCompressionTypeTest.java:
##########
@@ -0,0 +1,78 @@
+/**
+ * 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.BeforeMethod;
+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.
+///
+/// Not thread-safe: every method mutates the process-global
[ForwardIndexConfig] default, so this class
+/// must not run in parallel with other tests, or with anything that builds
segments.
+public class ForwardIndexDefaultCompressionTypeTest {
Review Comment:
Partly taken. The two producer-side cases went into the existing
`ForwardIndexCreatorFactoryTest`, next to the assertions that already cover the
codec a creator reports, which is where they belong.
I kept `ForwardIndexDefaultCompressionTypeTest` separate. Its lifecycle is
not just a one-line reset: it mutates a process-global static in
`@BeforeMethod` and `@AfterMethod`, and now also captures the compiled-in value
in `@BeforeClass` before anything sets it. Folding that into
`ForwardIndexTypeTest` imposes the reset on every unrelated test there and
makes the `@BeforeClass` capture depend on no sibling test having moved the
static first. The two tests you note as covering pre-existing behavior are
gone: the redundant LZ4 one is replaced by the `@BeforeClass` capture, which
actually can fail.
--
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]
