arunkumarucet commented on code in PR #19797:
URL: https://github.com/apache/pinot/pull/19797#discussion_r4244899622
##########
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()) {
Review Comment:
Verified and fixed. `GzipDecompressor` holds one `Inflater` as an instance
field with an unsynchronized `reset()` / `setInput()` / `inflate()` sequence,
and `VarByteChunkForwardIndexReaderV4` builds one decompressor per reader (L75)
then passes that same instance into every `CompressedReaderContext` (L119) — so
the contexts share it, exactly as you describe.
`ForwardIndexConfig.setDefaultCompressionCodec` now rejects `GZIP` outright,
with the reasoning in its javadoc: naming it on one column is an existing
opt-in exposure, making it the default would put every raw non-metric column on
that path. Covered by `testGzipIsRejectedAsADefault`, and the rejection is in
`ForwardIndexConfig` rather than the parser so an embedder setting it directly
is guarded too.
I did not fix `GzipDecompressor` itself. A `ThreadLocal` or per-context
`Inflater` is the real repair and it fixes a live bug for anyone already using
`compressionCodec: GZIP`, but it is a different change with a different blast
radius and I would rather it not ride in on a config addition. Happy to raise
it separately.
##########
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
+ /// what gets written to disk.
+ public static ChunkCompressionType getDefaultCompressionType() {
+ return _defaultCompressionType;
+ }
+
+ public static void setDefaultCompressionType(ChunkCompressionType
defaultCompressionType) {
Review Comment:
Agreed, and moved. Validation now lives in
`ForwardIndexConfig.setDefaultCompressionCodec(CompressionCodec)`: it checks
`isApplicableToRawIndex()`, rejects `GZIP`, and maps through the shared helper.
`ServiceStartableUtils` only parses the string and reports what was rejected.
The unvalidated `setDefaultCompressionType(ChunkCompressionType)` is gone
rather than kept alongside it. It was introduced in this PR, so there is no
compatibility cost, and leaving it would have preserved exactly the hole you
describe — `null`, `DELTA`, `LZ4_LENGTH_PREFIXED` all reachable from outside
pinot-common.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/openstruct/OpenStructColumnSplitter.java:
##########
@@ -567,7 +567,8 @@ private void writeDenseKeyColumn(String key)
// Reconcile dictionary + forward encoding with the final decision
(mirrors BaseSegmentCreator.adaptConfig);
// ForwardIndexCreatorFactory selects dict-vs-raw from the forward
config's EncodingType. A compression codec
- // applies only to the raw forward format (LZ4 preserves the dense child's
current on-disk layout); attaching
+ // applies only to the raw forward format (LZ4 preserves the dense child's
current on-disk layout, and is
+ // pinned here regardless of the cluster-wide default compression codec);
attaching
Review Comment:
You are right, and I had only half-fixed this: last round I annotated the
splitter's own comment but left `OpenStructIndexType`'s claim standing.
`FIXED_FORWARD_INDEX` now reads "the splitter fixes materialized keys to a
dictionary-encoded or LZ4 raw forward index" rather than "materialized keys
always use ...", which is the word that stopped being true. The validation
comment and the `validatePerKeyForwardIndex` javadoc both say the LZ4 is the
splitter's and that a dictionary-to-raw conversion on reload takes the
reloading component's default instead. Three test assertions pinned the old
message and were updated with it.
Worth noting a second copy of that sentence lived in
`OpenStructColumnSplitter` itself, line-split across two string literals, which
is why my first pass missed it — the tests caught it.
I have not pinned the reload path. Making it match means the open-struct
reload route carrying its codec explicitly, which is a change owned by that
feature rather than by this config, and I would rather not silently alter
open-struct segment output from here. The regression test you suggest belongs
with that change.
##########
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.
Review Comment:
Corrected. The sentence now reads that a reload ignores the default when
*deciding* whether to rewrite, so nothing reconciles the two on its own, but
that a rewrite triggered for another reason — dictionary-to-raw conversion,
codecSpec removal, re-enabling a disabled forward index, a new derived or
default column — does write the reloading component's current default.
--
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]
