xiangfu0 commented on code in PR #20116:
URL: https://github.com/apache/pinot/pull/20116#discussion_r4440605043
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/retention/RetentionManagerTest.java:
##########
@@ -560,6 +563,56 @@ private RetentionManager
createRetentionManagerWithHybridRetentionOverride(Table
return retentionManager;
}
+ @DataProvider(name = "hybridTableRetentionStrategyOverrides")
+ public Object[][] hybridTableRetentionStrategyOverrides() {
+ // tableLevel (null means omitted; null and DEFAULT fall back to
clusterLevel), clusterLevel, expectHybridPath
+ return new Object[][]{
+ {null, false, false},
+ {null, true, true},
+ {Enablement.DEFAULT, false, false},
+ {Enablement.DEFAULT, true, true},
+ {Enablement.ENABLE, false, true},
+ {Enablement.ENABLE, true, true},
+ {Enablement.DISABLE, false, false},
+ {Enablement.DISABLE, true, false},
+ };
+ }
+
+ @Test(dataProvider = "hybridTableRetentionStrategyOverrides")
+ public void
testTableLevelHybridRetentionStrategyOverridesClusterConfig(@Nullable
Enablement tableLevel,
+ boolean clusterLevel, boolean expectHybridPath) {
+ int replicaCount = 1;
+ TableConfig realtimeTableConfig = createRealtimeTableConfig1(replicaCount);
+ if (tableLevel != null) {
+
realtimeTableConfig.getValidationConfig().setHybridTableRetentionStrategy(tableLevel);
+ }
+
+ PinotHelixResourceManager resourceManager =
mock(PinotHelixResourceManager.class);
+ when(resourceManager.getHelixAdmin()).thenReturn(mock(HelixAdmin.class));
+ setupSizeRetentionOfflineCounterpart(resourceManager);
+
+ ControllerConf conf = new ControllerConf();
+ conf.setProperty(ControllerConf.ENABLE_HYBRID_TABLE_RETENTION_STRATEGY,
clusterLevel);
+
+ RetentionManager retentionManager =
spy(createSizeRetentionManager(realtimeTableConfig, List.of(), resourceManager,
+ mock(ControllerMetrics.class), conf, mock(BrokerServiceHelper.class)));
+
+ // Only the retention strategy selection is under test, so stub out both
branches
+ doNothing().when(retentionManager).manageRetentionForHybridTable(any(),
any());
+ doNothing().when(retentionManager).manageRetentionForRealtimeTable(any(),
any(), anyInt(), any());
+
+ retentionManager.processTable(REALTIME_TABLE_NAME);
+
+ if (expectHybridPath) {
+ verify(retentionManager).manageRetentionForHybridTable(any(), any());
+ verify(retentionManager, never()).manageRetentionForRealtimeTable(any(),
any(), anyInt(), any());
+ } else {
+ verify(retentionManager, never()).manageRetentionForHybridTable(any(),
any());
+ verify(retentionManager).manageRetentionForRealtimeTable(any(), any(),
anyInt(), any());
+ verify(resourceManager, never()).getOfflineTableConfig(anyString());
Review Comment:
**[MAJOR] Stale assertion; 4 of the 8 cases fail.**
`manageRetentionForTable` now always fetches the OFFLINE config
(`RetentionManager.java:367`), because the hook needs it as input. This
`never()` check therefore fails cases `[0]`, `[2]`, `[6]` and `[7]`, both
locally and in CI Unit Test Set 2. With this line removed, I get 67/67 passing.
Please drop it, or change it to
`verify(resourceManager).getOfflineTableConfig(TEST_TABLE_NAME)`.
_AI-generated review via IntentLab · Model: claude-opus-5-5._
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java:
##########
@@ -377,7 +383,8 @@ private void manageRetentionForTable(TableConfig
tableConfig) {
/// runtime through cluster config.
protected boolean shouldEnableHybridTableRetentionStrategy(TableConfig
realtimeTableConfig,
TableConfig offlineTableConfig) {
- return _isHybridTableRetentionStrategyEnabled;
+ return
realtimeTableConfig.getValidationConfig().getHybridTableRetentionStrategy()
Review Comment:
**[MINOR] Javadoc is stale, and overrides can bypass a table-level
`DISABLE`.** The javadoc above still says the default returns the cluster flag.
The default no longer returns the cluster flag directly; it resolves the
table-level `Enablement` first. Also, a subclass that overrides this hook
silently drops the table-level `DISABLE` opt-out. Consider resolving
`ENABLE`/`DISABLE` in `manageRetentionForTable` before calling the hook, or at
least document that overrides must honor it.
_AI-generated review via IntentLab · Model: claude-opus-5-5._
--
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]
