xiangfu0 commented on code in PR #20168:
URL: https://github.com/apache/pinot/pull/20168#discussion_r4480818554
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -608,6 +608,17 @@ static void validateIngestionConfig(TableConfig
tableConfig, Schema schema,
} else {
LOGGER.warn("It's not recommended to create pauseless tables with
replication 1 for stability reasons.");
}
+ // Pauseless consumption needs a row or size flush threshold, which
stops all replicas at the same offset. Each
+ // replica checks a time threshold on its own (a restarted replica
also extends it), so replicas can stop at
+ // different offsets. A replica past the committed end offset can only
download the segment, so if the
+ // committer fails before the upload, no replica has a copy.
+ for (Map<String, String> streamConfigMap : streamConfigMaps) {
+ for (String key :
List.of(StreamConfigProperties.SEGMENT_FLUSH_THRESHOLD_TIME,
+ StreamConfigProperties.SEGMENT_FLUSH_THRESHOLD_TIME +
StreamConfigProperties.LLC_SUFFIX)) {
+ Preconditions.checkState(!streamConfigMap.containsKey(key),
Review Comment:
This check ignores `existingTableConfig`, so the impact is wider than "the
next config update must remove the key." `PinotSchemaRestletResource`
revalidates every attached table config through
`SchemaUtils.validateCompatibilityWithTableConfig`, which calls
`validate(tableConfig, schema, null, tableConfig)`. As a result, a schema
change such as adding a column now fails for an existing pauseless table that
sets this key.
I reproduced this locally at this SHA: `SchemaUtils.validate(newSchema,
List.of(existingPauselessCfg))` fails with "Schema is incompatible with
tableConfig ... realtime.segment.flush.threshold.time is not supported with
pauseless consumption enabled". Either grandfather the key when
`existingTableConfig` already has it with pauseless enabled, as the transform
checks do, or state the schema-update impact in the PR description and upgrade
notes.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -608,6 +608,17 @@ static void validateIngestionConfig(TableConfig
tableConfig, Schema schema,
} else {
LOGGER.warn("It's not recommended to create pauseless tables with
replication 1 for stability reasons.");
}
+ // Pauseless consumption needs a row or size flush threshold, which
stops all replicas at the same offset. Each
Review Comment:
The default time threshold still applies when this key is absent.
`StreamConfig.extractFlushThresholdTimeMillis` returns
`DEFAULT_FLUSH_THRESHOLD_TIME_MILLIS` (6h), and
`RealtimeSegmentDataManager.endCriteriaReached` still stops each replica with
`TIME_LIMIT`. Nothing there special-cases pauseless. So on a low-volume
partition, a pauseless table that omits the key can still have replicas stop at
different offsets. That is the failure mode this PR describes.
This check also blocks one existing mitigation: setting a larger time value
(e.g. `7d`) to make time-based stops rare. Even an explicit `6h` is rejected,
though it behaves the same as the default. Consider handling this on the
consumption side, such as ignoring or raising the time limit for pauseless
tables, or validating the value instead of the key's presence. At minimum, the
comment should not say that a row or size threshold alone stops all replicas at
the same offset.
--
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]
