Jackie-Jiang commented on code in PR #19710:
URL: https://github.com/apache/pinot/pull/19710#discussion_r4187948743
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1225,6 +1228,30 @@ static void validateUpsertAndDedupConfig(TableConfig
tableConfig, Schema schema,
}
}
+ /// Rejects consuming the next segment during a download on tables that
revert upsert metadata in PROTECTED mode,
+ /// because the next segment's snapshot would run before the revert and miss
the rows it restores.
+ @VisibleForTesting
+ static void validateConsumptionDuringUpsertRevert(TableConfig tableConfig) {
+ if (tableConfig.getTableType() != TableType.REALTIME ||
!isTableTypeInconsistentDuringConsumption(tableConfig)
+ ||
ConsumingSegmentConsistencyModeListener.getInstance().getConsistencyMode()
+ != ConsumingSegmentConsistencyModeListener.Mode.PROTECTED) {
+ return;
+ }
+ IngestionConfig ingestionConfig = tableConfig.getIngestionConfig();
+ StreamIngestionConfig streamIngestionConfig =
+ ingestionConfig != null ? ingestionConfig.getStreamIngestionConfig() :
null;
+ ParallelSegmentConsumptionPolicy policy =
+ streamIngestionConfig != null ?
streamIngestionConfig.getParallelSegmentConsumptionPolicy() : null;
+ // ALLOW_ALWAYS and ALLOW_DURING_DOWNLOAD_ONLY both allow it, and so does
the deprecated flag when no policy is set
+ boolean consumesDuringDownload = policy != null ?
policy.isAllowedDuringDownload()
+ :
tableConfig.getUpsertConfig().isAllowPartialUpsertConsumptionDuringCommit();
+ Preconditions.checkState(!consumesDuringDownload,
+ "%s lets the next segment consume during a segment download, but
tables with partial upsert, "
+ + "dropOutOfOrderRecord or outOfOrderRecordColumn revert upsert
metadata in PROTECTED consistency mode. "
+ + "Set parallelSegmentConsumptionPolicy to DISALLOW_ALWAYS or
ALLOW_DURING_BUILD_ONLY",
Review Comment:
Non-blocking follow-up: `ALLOW_DURING_BUILD_ONLY` is not universally safe
here. `buildSegmentInternal(false)` releases the consumer semaphore before
building, and a CRC mismatch can then make `goOnlineFromConsuming()` fall back
to downloading the committed copy. The next consumer can already have taken its
snapshot by that point.
If the committed copy omits a key accepted locally (for example, because
replicas made different `dropOutOfOrderRecord` decisions), replacement restores
that key's previous immutable row after its snapshot. The restored segment is
not marked for another snapshot, so restart with preload can still lose the
row. The new warning/meter also misses this path because
`isAllowedDuringDownload()` is false.
The underlying race predates this PR, but this validation explicitly accepts
and recommends this policy as a remedy. We should cover the build-to-download
fallback and either retain the semaphore until the local build is accepted or
handle this case in the validation/runtime protection.
--
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]
