xiangfu0 commented on code in PR #20221:
URL: https://github.com/apache/pinot/pull/20221#discussion_r4535445524
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -622,20 +608,6 @@ static void validateIngestionConfig(TableConfig
tableConfig, Schema schema,
} else {
LOGGER.warn("It's not recommended to create pauseless tables with
replication 1 for stability reasons.");
}
Review Comment:
This re-allows the hazard the removed comment described. Each replica
derives `_consumeEndTime` from its own clock
(`RealtimeSegmentDataManager#setConsumeEndTime`), and a replica past the
committed end offset can only download
(`RealtimeSegmentDataManager.java:1840`). Could the PR description say why
that's acceptable now (e.g., a server-side fix or a planned follow-up)? The 6h
default still carries the same exposure either way.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -528,26 +527,13 @@ private static boolean isValidPeerDownloadScheme(String
peerSegmentDownloadSchem
||
CommonConstants.HTTPS_PROTOCOL.equalsIgnoreCase(peerSegmentDownloadScheme);
}
- /// Returns whether the stored table config is already pauseless and sets
`key` to `value` in one of its stream
- /// configs. This value stays valid, so that updates to an existing table
and to its schema keep working.
- private static boolean hasSamePauselessTimeFlushThreshold(@Nullable
TableConfig existingTableConfig, String key,
- String value) {
- if (existingTableConfig == null ||
!PauselessConsumptionUtils.isPauselessEnabled(existingTableConfig)) {
- return false;
- }
- // Pauseless consumption is enabled in the stream ingestion config, so the
ingestion config is not null
- return
existingTableConfig.getIngestionConfig().getStreamIngestionConfig().getStreamConfigMaps().stream()
- .anyMatch(streamConfigMap -> value.equals(streamConfigMap.get(key)));
- }
-
/// Validates the table's [IngestionConfig] together with the closely
related metrics-aggregation config, covering:
/// - Metrics aggregation (both the `aggregateMetrics` flag and ingestion
`aggregationConfigs`), delegated to
/// [#validateMetricsAggregation].
/// - Batch ingestion: each batch config map is well-formed, and a dimension
table has batch ingestion configured
/// with `REFRESH` segment ingestion type.
/// - Stream ingestion: streams are declared in only one place, at least one
stream is present, and pauseless
- /// consumption has a valid `peerSegmentDownloadScheme` and no time flush
threshold, except the unchanged value of
- /// a stored pauseless table.
+ /// consumption has a valid `peerSegmentDownloadScheme`.
Review Comment:
The Javadoc is correctly restored. Note that pinot-contrib/pinot-docs#1170
(merged) added the rejection to `operate-pinot/pauseless-consumption.md`.
Please revert that docs change too, or the guide will say these configs are
rejected.
--
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]
