KKcorps commented on code in PR #20010:
URL: https://github.com/apache/pinot/pull/20010#discussion_r4387725227
##########
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:
After `manageRetentionForTable` deletes, this context still returns the old
ZK list, so a hook that runs after `super` would see deleted segments as live,
and size retention holds it in memory next to its own copy. Could we clear the
memoized list after that step, or at least say in the doc that hooks must read
before calling `super`?
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/retention/RetentionManagerTest.java:
##########
@@ -1858,4 +1861,77 @@ private void
setupSizeRetentionLineage(PinotHelixResourceManager resourceManager
when(resourceManager.getPropertyStore().get(anyString(), any(Stat.class),
eq(AccessOption.PERSISTENT)))
.thenReturn(lineage.toZNRecord());
}
+
+ /// Per-table work added by a subclass must not add ZK or deep-store reads.
The subclass reads through the run
+ /// context before the built-in steps run, and the whole call sees one ZK
metadata read and one listing.
+ @Test
+ public void testRunContextSharesReadsWithSubclassPerTableWork() {
Review Comment:
Could we add a case where the hook's first listing throws? It would pin that
the base retries it and still runs time retention, which is the part a deep
store check leans on most.
##########
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.
Review Comment:
Small one: the base only lists deep store when untracked deletion is on, and
that is off by default, so a hook that lists adds one listing per table per
run. Maybe say it shares the reads the base already makes, instead of zero
extra?
--
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]
