Jackie-Jiang commented on code in PR #20057:
URL: https://github.com/apache/pinot/pull/20057#discussion_r4417967088
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java:
##########
@@ -184,6 +186,15 @@ public void setLineageEntryCleanupRetentionPeriod(String
lineageEntryCleanupRete
_lineageEntryCleanupRetentionPeriod = lineageEntryCleanupRetentionPeriod;
}
+ @Nullable
+ public Boolean getEnableHybridTableRetentionStrategy() {
+ return _enableHybridTableRetentionStrategy;
+ }
+
+ public void setEnableHybridTableRetentionStrategy(Boolean
enableHybridTableRetentionStrategy) {
Review Comment:
Please document the override contract here: it applies to the REALTIME side
of a hybrid table, and `DEFAULT` inherits the cluster setting after the
proposed `Enablement` change.
The current `Boolean` setter intentionally accepts null but lacks
`@Nullable`. If the replacement setter also accepts null, annotate the
parameter and normalize it to `Enablement.DEFAULT` so the null contract is
explicit.
##########
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);
Review Comment:
The string literals currently concatenate into `realtime tables,ignoring`,
without a space. Please also use the usual wrapping style, with the first
literal on the call line and the argument alongside the completed format string:
```java
LOGGER.warn("Hybrid table retention strategy is only applicable to realtime
tables, "
+ "ignoring it for offline table: {}", tableNameWithType);
```
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/retention/RetentionManagerTest.java:
##########
@@ -515,6 +516,49 @@ public void testManageRetentionForHybridTable() {
verify(mockPinotHelixResourceManager,
times(1)).deleteSegments(eq(realtimeTableName), anyList());
}
+ @DataProvider(name = "hybridTableRetentionStrategyOverrides")
+ public Boolean[][] hybridTableRetentionStrategyOverrides() {
+ // tableLevel (null falls back to clusterLevel), clusterLevel,
expectHybridPath
+ return new Boolean[][]{
+ {null, false, false},
+ {null, true, true},
+ {true, false, true},
+ {false, true, false},
+ };
+ }
+
+ @Test(dataProvider = "hybridTableRetentionStrategyOverrides")
+ public void
testTableLevelHybridRetentionStrategyOverridesClusterConfig(Boolean tableLevel,
boolean clusterLevel,
+ boolean expectHybridPath) {
+ final int replicaCount = 1;
Review Comment:
Nit: please remove `final` from this local variable (`int replicaCount =
1;`) to follow Pinot's local-variable convention.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java:
##########
@@ -31,6 +32,7 @@ public class SegmentsValidationAndRetentionConfig extends
BaseJsonConfig {
private String _deletedSegmentsRetentionPeriod;
private String _replacedSegmentsRetentionPeriod;
private String _lineageEntryCleanupRetentionPeriod;
+ private Boolean _enableHybridTableRetentionStrategy;
Review Comment:
Following up on the earlier `Enablement` discussion: I prefer using
`Enablement` here. The setting has three explicit states, and we already have
an enum for this override pattern. Please name the field
`_hybridTableRetentionStrategy`, initialize it to `Enablement.DEFAULT`, and
expose it through `getHybridTableRetentionStrategy()`. Resolution can then use
`.isEnabled(_isHybridTableRetentionStrategyEnabled)`.
We can exclude the default value from serialization with a Jackson inclusion
annotation, so avoiding an extra field in existing table configs does not
require keeping a nullable `Boolean`. Please update the serialization and
override tests to cover `ENABLE`, `DISABLE`, `DEFAULT`, and an omitted field.
Keeping `Strategy` in the name matters: `DISABLE` selects ordinary realtime
retention; it does not disable retention altogether.
--
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]
