KKcorps commented on code in PR #19710:
URL: https://github.com/apache/pinot/pull/19710#discussion_r4193968098
##########
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:
Good catch, thanks! Validation now accepts only `DISALLOW_ALWAYS` here
(pauseless keeps its build-only default), and the meter also fires when a
failed or CRC-mismatched build falls back to download, with a test for that
path.
@deepthi912 does this direction look fine to you? The other option is to
keep allowing `ALLOW_DURING_BUILD_ONLY` and skip the CRC-mismatch download for
it, like we already do for pauseless.
--
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]
