9aman commented on code in PR #20010:
URL: https://github.com/apache/pinot/pull/20010#discussion_r4562730487
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java:
##########
@@ -664,6 +665,81 @@ private String extractSegmentName(@Nullable String
filePath) {
return segmentName;
}
+ /// Lists the table data dir (`<dataDir>/<rawTableName>/`) without
recursion. Returns null when the directory does
+ /// not exist: the data dir is created when the first segment is pushed, so
it is legitimately absent for a table
+ /// that has never had a segment in deep store, and listing a non-existent
directory fails on most file systems.
+ @Nullable
+ protected List<FileMetadata> listDeepStoreTableDir(URI tableDataUri)
+ throws IOException {
+ PinotFS pinotFS = PinotFSFactory.create(tableDataUri.getScheme());
+ if (!pinotFS.exists(tableDataUri)) {
+ return null;
+ }
+ long startTimeMs = System.currentTimeMillis();
+ List<FileMetadata> deepstoreFiles =
pinotFS.listFilesWithMetadata(tableDataUri, false);
+ LOGGER.info("Found: {} segments in deepstore dir: {}. Time taken to list
segments: {} ms", deepstoreFiles.size(),
+ tableDataUri, System.currentTimeMillis() - startTimeMs);
+ return deepstoreFiles;
+ }
+
+ /// Per-table state of one run. Memoizes the segment ZK metadata list and
the deep-store directory listing, the two
+ /// reads that dominate the cost on a large table, so each happens at most
once per table per run and every consumer
+ /// in the same `processTable` call sees the same snapshot. A context lives
for one `processTable` call only, so the
+ /// memory held for one table is released before the next table is
processed. Not thread-safe: the task processes
+ /// tables sequentially under its run lock.
+ public final class TableRunContext {
+ private final String _tableNameWithType;
+ private final String _rawTableName;
+ @Nullable
+ private URI _tableDataUri;
+ @Nullable
+ private List<SegmentZKMetadata> _segmentsZKMetadata;
+ private boolean _deepStoreListed;
+ @Nullable
+ private List<FileMetadata> _deepStoreFiles;
+
+ TableRunContext(String tableNameWithType) {
+ _tableNameWithType = tableNameWithType;
+ _rawTableName = TableNameBuilder.extractRawTableName(tableNameWithType);
+ }
+
+ public String getTableNameWithType() {
+ return _tableNameWithType;
+ }
+
+ public String getRawTableName() {
+ return _rawTableName;
+ }
+
+ /// `<dataDir>/<rawTableName>`, the directory that holds the deep-store
copies of the table's segments.
+ public URI getTableDataUri() {
+ if (_tableDataUri == null) {
+ _tableDataUri =
URIUtils.getUri(_pinotHelixResourceManager.getDataDir(), _rawTableName);
+ }
+ return _tableDataUri;
+ }
+
+ /// Segment ZK metadata of the typed table, read on the first call and
reused afterwards.
+ public List<SegmentZKMetadata> getSegmentsZKMetadata() {
+ if (_segmentsZKMetadata == null) {
Review Comment:
Will add the comment with the next change.
##########
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:
Done in a80eb94: the context is released after time retention, so a read
after `super` is fresh.
##########
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:
Done: the doc now says the context shares the reads the base makes.
##########
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:
Added `testFailedListingIsRetriedAndTimeRetentionStillRuns`.
##########
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:
Done: the context is released right after `manageRetentionForTable`.
--
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]
