Jackie-Jiang commented on code in PR #20057:
URL: https://github.com/apache/pinot/pull/20057#discussion_r4418218830
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java:
##########
@@ -354,13 +355,22 @@ private void manageRetentionForTable(TableConfig
tableConfig) {
// Scan all segment ZK metadata and purge segments if necessary
if (TableNameBuilder.isOfflineTableResource(tableNameWithType)) {
+ if (validationConfig.getEnableHybridTableRetentionStrategy() != null) {
+ LOGGER.warn(
+ "enableHybridTableRetentionStrategy is only applicable to realtime
tables,"
+ + "ignoring it for offline table: {}",
+ tableNameWithType);
+ }
manageRetentionForOfflineTable(tableNameWithType, retentionStrategy,
untrackedSegmentsDeletionBatchSize,
untrackedSegmentsRetentionStrategy);
} else {
String rawTableName =
TableNameBuilder.extractRawTableName(tableNameWithType);
TableConfig offlineTableConfig =
_pinotHelixResourceManager.getOfflineTableConfig(rawTableName);
Review Comment:
We can skip reading the OFFLINE table config when the effective hybrid
retention strategy is disabled. Please resolve the table/cluster setting first
and only fetch this config when enabled; it is only used by the hybrid branch.
This avoids an unnecessary property-store lookup and config deserialization on
the ordinary realtime-retention path.
With the proposed `Enablement` change:
```java
boolean hybridRetentionEnabled =
validationConfig.getHybridTableRetentionStrategy()
.isEnabled(_isHybridTableRetentionStrategyEnabled);
TableConfig offlineTableConfig = hybridRetentionEnabled
?
_pinotHelixResourceManager.getOfflineTableConfig(TableNameBuilder.extractRawTableName(tableNameWithType))
: null;
```
Then use hybrid retention when `offlineTableConfig != null`, otherwise
ordinary realtime retention. The override test can also verify that
`getOfflineTableConfig()` is never called when the effective setting is
disabled.
--
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]
