Jackie-Jiang commented on code in PR #20116:
URL: https://github.com/apache/pinot/pull/20116#discussion_r4448189537
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java:
##########
@@ -184,6 +192,22 @@ public void setLineageEntryCleanupRetentionPeriod(String
lineageEntryCleanupRete
_lineageEntryCleanupRetentionPeriod = lineageEntryCleanupRetentionPeriod;
}
+ /// Whether to use the hybrid table retention strategy for this table. Only
applicable to realtime tables. DEFAULT
+ /// falls back to the controller-level config
`controller.enable.hybrid.table.retention.strategy`.
+ ///
+ /// When enabled and an OFFLINE counterpart exists, realtime segments are
retained until the OFFLINE time boundary
+ /// moves past them instead of by `retentionTimeValue`, so data is not
dropped before the OFFLINE table covers it.
Review Comment:
**[MINOR documentation] Scope this guarantee to time-based retention.**
The statement that data is retained until OFFLINE coverage is too broad.
`RetentionManager.processTable()` still runs independent size retention
afterward (lines 127–133), and `getRetentionSize()` explicitly allows deleting
REALTIME data newer than the OFFLINE boundary.
Please describe this setting as selecting the time-based retention policy
and link `getRetentionSize()`. Also state the existing prerequisites/fallbacks:
valid `retentionTimeUnit` and `retentionTimeValue` are still needed; `DISABLE`
selects ordinary realtime retention rather than disabling retention; and
without an OFFLINE counterpart, ordinary realtime retention is used. These are
existing behaviors to clarify, not new runtime regressions.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/SegmentsValidationAndRetentionConfig.java:
##########
@@ -184,6 +192,22 @@ public void setLineageEntryCleanupRetentionPeriod(String
lineageEntryCleanupRete
_lineageEntryCleanupRetentionPeriod = lineageEntryCleanupRetentionPeriod;
}
+ /// Whether to use the hybrid table retention strategy for this table. Only
applicable to realtime tables. DEFAULT
+ /// falls back to the controller-level config
`controller.enable.hybrid.table.retention.strategy`.
+ ///
+ /// When enabled and an OFFLINE counterpart exists, realtime segments are
retained until the OFFLINE time boundary
+ /// moves past them instead of by `retentionTimeValue`, so data is not
dropped before the OFFLINE table covers it.
+ // Ignored for serialization so that DEFAULT is omitted, see the field
+ @JsonIgnore
+ public Enablement getHybridTableRetentionStrategy() {
+ return _hybridTableRetentionStrategy == null ? Enablement.DEFAULT :
_hybridTableRetentionStrategy;
+ }
+
+ public void setHybridTableRetentionStrategy(@Nullable Enablement
hybridTableRetentionStrategy) {
+ _hybridTableRetentionStrategy =
+ hybridTableRetentionStrategy == Enablement.DEFAULT ? null :
hybridTableRetentionStrategy;
Review Comment:
**[MINOR] Normalize explicit JSON `DEFAULT` through this setter.**
With the current `@JsonIgnore` getter and separately annotated field,
Jackson bypasses this setter and writes the field directly. An isolated probe
with this annotation layout and Jackson 2.22.4 reproduces:
- Java setter `DEFAULT` → `{}`
- JSON `{"hybridTableRetentionStrategy":"DEFAULT"}` → the same field remains
serialized.
The effective retention strategy is correct, but this violates the
default-omission contract and makes an explicit-DEFAULT config compare unequal
to an otherwise identical unset config, because `BaseJsonConfig.equals()`
compares serialized JSON.
Please add `@JsonProperty("hybridTableRetentionStrategy")` to the setter and
cover explicit JSON DEFAULT. This fixes the probe without changing
ENABLE/DISABLE/null behavior. Keeping the nullable backing field and non-null
effective getter is reasonable; a custom filter is unnecessary, and class-level
`NON_DEFAULT` could change unrelated serialized fields.
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/config/TableConfigSerDeUtilsTest.java:
##########
@@ -376,6 +377,57 @@ public void testSerDe()
assertEquals(tunerConfigToCompare.getName(), name);
assertEquals(tunerConfigToCompare.getTunerProperties(), props);
}
+ {
+ // With hybrid table retention strategy
Review Comment:
**[Optional cleanup] Give these serialization cases a focused test.**
This new 51-line block extends an already large `testSerDe()` and inherits
its accumulated builder settings. Consider extracting it into a separate test
method in this class with a fresh REALTIME config. The current OFFLINE fixture
does not invalidate serialization coverage; a REALTIME fixture simply makes the
setting's intended use clearer.
The important regression case is raw JSON
`"hybridTableRetentionStrategy":"DEFAULT"`: the current loop calls the Java
setter first, which omits the field before deserialization and misses the
normalization bug. Assert omission after parsing, equality with an unset
config, and normalization through the ZNRecord round trip. Keep the existing
ENABLE/DISABLE/null/omitted cases.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java:
##########
@@ -354,6 +355,11 @@ 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, "
+ + "ignoring it for offline table: {}", tableNameWithType);
Review Comment:
**[Nit] Use the usual log-call wrapping.**
The missing space is fixed. The remaining cosmetic cleanup is to put the
first literal on the call line and align the continuation:
```java
LOGGER.warn("hybridTableRetentionStrategy is only applicable to realtime
tables, "
+ "ignoring it for offline table: {}", tableNameWithType);
```
--
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]
