xiangfu0 commented on code in PR #20010:
URL: https://github.com/apache/pinot/pull/20010#discussion_r4508410586
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java:
##########
@@ -120,10 +120,17 @@ protected void processTable(String tableNameWithType) {
return;
}
+ processTable(tableConfig, new TableRunContext(tableNameWithType));
+ }
+
+ /// Runs the per-table work of one run. `context` memoizes the segment ZK
metadata list and the deep-store directory
+ /// listing, so each is read at most once per table per run. Subclasses that
add per-table work override this method
+ /// and read through the same context, which keeps the added cost at zero
extra ZK reads and zero extra listings.
+ protected void processTable(TableConfig tableConfig, TableRunContext
context) {
// Manage normal table retention except segment lineage cleanup.
// The reason of separating the logic is that REFRESH only table will be
skipped in the first part,
// whereas the segment lineage cleanup needs to be handled.
- manageRetentionForTable(tableConfig);
+ manageRetentionForTable(tableConfig, context);
Review Comment:
`context` stays live across the later
`manageSizeBasedRetention(tableConfig)` step (L139), so the first ZK metadata
list and the deep-store listing remain reachable while
`manageSizeBasedRetention` loads its own fresh copy. For a size-retention
table, that roughly doubles peak heap compared with master. In a local harness
with 50k segments, live heap after GC during size retention went from ~25 MB to
~81 MB, with ZK reads unchanged at 2. Dropping the memoized state after
`manageRetentionForTable` would fix this, and it would also fix the
stale-after-delete issue KKcorps raised.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java:
##########
@@ -120,10 +120,17 @@ protected void processTable(String tableNameWithType) {
return;
}
+ processTable(tableConfig, new TableRunContext(tableNameWithType));
+ }
+
+ /// Runs the per-table work of one run. `context` memoizes the segment ZK
metadata list and the deep-store directory
+ /// listing, so each is read at most once per table per run. Subclasses that
add per-table work override this method
Review Comment:
`manageRetentionForHybridTable` (L452) and `manageSizeBasedRetention` still
read ZK directly. A subclass that reads the context on a hybrid realtime table
with the hybrid strategy on, or on any table with `retentionSize`, therefore
adds a ZK read instead of zero. Could the doc scope "at most once" to the
time/untracked retention step?
--
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]
