xiangfu0 commented on code in PR #20088:
URL: https://github.com/apache/pinot/pull/20088#discussion_r4412189176
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java:
##########
@@ -370,6 +370,11 @@ private void manageRetentionForTable(TableConfig
tableConfig) {
}
}
+ protected boolean shouldEnableHybridTableRetentionStrategy(TableConfig
realtimeTableConfig,
Review Comment:
**[MAJOR] Test coverage:** nothing tests this new extension point. No test
currently checks the routing in `manageRetentionForTable` either;
`testManageRetentionForHybridTable` calls the hybrid method directly. A
refactor that inlines the flag again, or calls the hook before the null check,
would break downstream overrides without failing any test. Could you add a
`RetentionManagerTest` case with a subclass that returns `true` while
`controller.enable.hybrid.table.retention.strategy=false`? It should verify
that `processTable` takes the hybrid path, that the hook isn't called when
there's no OFFLINE config, and that the hook receives the right (realtime,
offline) configs.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java:
##########
@@ -370,6 +370,11 @@ private void manageRetentionForTable(TableConfig
tableConfig) {
}
}
+ protected boolean shouldEnableHybridTableRetentionStrategy(TableConfig
realtimeTableConfig,
+ TableConfig offlineTableConfig) {
Review Comment:
**[MINOR] Hook contract:** since this is a subclass hook on the deletion
path, please add a `///` doc covering:
- It's called once per run, and only for REALTIME tables that have an
OFFLINE counterpart.
- Returning `true` replaces the realtime retention path. COMPLETED segments
that end before the OFFLINE time boundary are deleted, and time retention,
stale IN_PROGRESS LLC cleanup and untracked-segment deletion are skipped (see
the TODO above).
- The default returns `controller.enable.hybrid.table.retention.strategy`,
which can be updated at runtime.
- If the hook throws, lineage cleanup and size retention for that table are
skipped for this run.
--
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]
