[GitHub] [pinot] sajjad-moradi commented on a diff in pull request #10303: Add excludeReplacedSegments param into table metadata API

["sajjad-moradi (via GitHub)" <gi...@apache.org>]

sajjad-moradi commented on code in PR #10303:
URL: https://github.com/apache/pinot/pull/10303#discussion_r1113889192


##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -265,13 +269,31 @@ public boolean isSegmentDeletedRecently(String segmentName) {
 
   @Override
   public List<SegmentDataManager> acquireAllSegments() {
+    return acquireAllSegments(false);
+  }
+
+  @Override
+  public List<SegmentDataManager> acquireAllSegments(boolean excludeReplacedSegments) {
     List<SegmentDataManager> segmentDataManagers = new ArrayList<>();
     for (SegmentDataManager segmentDataManager : _segmentDataManagerMap.values()) {
       if (segmentDataManager.increaseReferenceCount()) {
         segmentDataManagers.add(segmentDataManager);
       }
     }
-    return segmentDataManagers;
+    // Fetch the segment lineage metadata, and conditionally filter out replaced segments based on segment lineage.
+    SegmentLineage segmentLineage = SegmentLineageAccessHelper.getSegmentLineage(_propertyStore, _tableNameWithType);
+    if (segmentLineage == null || !excludeReplacedSegments) {
+      return segmentDataManagers;
+    } else {
+      Set<String> replacedSegments = SegmentLineageUtils.getReplacedSegments(segmentLineage);
+      List<SegmentDataManager> filteredSegmentDataManagers = new ArrayList<>();
+      for (SegmentDataManager segmentDataManager : segmentDataManagers) {
+        if (!replacedSegments.contains(segmentDataManager.getSegmentName())) {
+          filteredSegmentDataManagers.add(segmentDataManager);
+        }
+      }
+      return filteredSegmentDataManagers;
+    }

Review Comment:
   There are a few problems here:
   1. If the input flag excludeReplacedSegments is false, we still get segment lineage from ZK. This is not desirable as it adds unnecessary calls to ZK.
   2. Here all segmentDataManagers are acquired, but in the return list, some of them are filtered out. In TablesResource, at the end of the endpoint, all of segmentDataManagers must be released, otherwise the memory resources for the filtered out segments will not be cleaned properly.
   3. Most importantly, the functionality added here doesn't belong to this class. It's something that's being used only in TablesResources. I suggest moving the new code to the calling endpoint and not change the interface.



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org