xiangfu0 commented on code in PR #20116:
URL: https://github.com/apache/pinot/pull/20116#discussion_r4437361891
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java:
##########
@@ -184,6 +192,19 @@ public void setLineageEntryCleanupRetentionPeriod(String
lineageEntryCleanupRete
_lineageEntryCleanupRetentionPeriod = lineageEntryCleanupRetentionPeriod;
}
+ /// Whether to use the hybrid table retention strategy for this table. Only
applicable to realtime tables. DEFAULT
Review Comment:
**[MINOR] Document what this switch deletes.** This is now a per-table
deletion switch, so the javadoc should spell out its effects:
- When an OFFLINE counterpart exists, it *replaces* realtime time retention:
COMPLETED realtime segments that end before the OFFLINE time boundary are
deleted, whatever the realtime `retentionTimeValue` says.
- The realtime table still needs a valid retention time and unit. Without
them, `manageRetentionForTable` returns before this setting is ever read.
_AI-generated review via IntentLab · Model: claude-opus-5-5._
##########
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.getHybridTableRetentionStrategy() !=
Enablement.DEFAULT) {
+ LOGGER.warn(
+ "hybridTableRetentionStrategy is only applicable to realtime
tables, "
Review Comment:
**[MINOR] Reject this misconfiguration when the config is written.** Right
now an OFFLINE table with this field set logs a WARN on every retention run and
the problem is never surfaced to whoever wrote the config. Consider rejecting a
non-`DEFAULT` value for OFFLINE tables in
`TableConfigUtils.validateRetentionConfig`, and keep this check only as a
fallback.
_AI-generated review via IntentLab · Model: claude-opus-5-5._
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/retention/RetentionManagerTest.java:
##########
@@ -515,6 +518,56 @@ public void testManageRetentionForHybridTable() {
verify(mockPinotHelixResourceManager,
times(1)).deleteSegments(eq(realtimeTableName), anyList());
}
+ @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);
Review Comment:
**[MINOR] Test gap.** Every case here has an OFFLINE counterpart. Please add
one where the table is `ENABLE` and `getOfflineTableConfig` returns `null`, and
assert that the realtime path runs. That locks in the null fallback on the
override path.
_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]
