xiangfu0 commented on code in PR #20057:
URL: https://github.com/apache/pinot/pull/20057#discussion_r4378828496
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -394,6 +394,9 @@ public static long getRandomInitialDelayInSeconds() {
// the controller as ready. When enabled,
controller.resource.utilization.checker.initial.delay is set to 0.
public static final String
RESOURCE_UTILIZATION_CHECKER_COLLECT_USAGE_AT_STARTUP =
"controller.resource.utilization.checker.collect.usage.at.startup";
+ /// @deprecated Use `segmentsConfig.enableHybridTableRetentionStrategy` in
the table config instead (see
+ ///
[org.apache.pinot.spi.config.table.SegmentsValidationAndRetentionConfig#getEnableHybridTableRetentionStrategy()]).
Review Comment:
Small nit: `since` marks the version where deprecation was introduced, which
is the current dev line `1.6.0` (that's the convention used elsewhere in the
repo). `1.7.0` reads more like the intended removal release. Suggest `since =
"1.6.0"`.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java:
##########
@@ -360,7 +361,10 @@ private void manageRetentionForTable(TableConfig
tableConfig) {
String rawTableName =
TableNameBuilder.extractRawTableName(tableNameWithType);
TableConfig offlineTableConfig =
_pinotHelixResourceManager.getOfflineTableConfig(rawTableName);
// hybrid table check should be performed before the realtime table
check.
- if (_isHybridTableRetentionStrategyEnabled && offlineTableConfig !=
null) {
+ boolean hybridTableRetentionStrategyEnabled =
+
Objects.requireNonNullElse(validationConfig.getEnableHybridTableRetentionStrategy(),
+ _isHybridTableRetentionStrategyEnabled);
+ if (hybridTableRetentionStrategyEnabled && offlineTableConfig != null) {
Review Comment:
This override is only read from the REALTIME table config (this branch is
realtime-only; the offline config here is used solely for the time boundary).
An operator who sets `enableHybridTableRetentionStrategy` on the OFFLINE config
of a hybrid table gets it silently ignored. Worth documenting the realtime-only
requirement, or logging when the offline config carries a conflicting non-null
value.
--
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]
