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

["snleee (via GitHub)" <gi...@apache.org>]

snleee commented on code in PR #10303:
URL: https://github.com/apache/pinot/pull/10303#discussion_r1113886403


##########
pinot-common/src/main/java/org/apache/pinot/common/lineage/SegmentLineageUtils.java:
##########
@@ -53,4 +54,19 @@ public static void filterSegmentsBasedOnLineageInPlace(Set<String> segments, Seg
       }
     }
   }
+
+  public static Set<String> getReplacedSegments(SegmentLineage segmentLineage) {

Review Comment:
   can we modify the `filterSegmentsBasedOnLineageInPlace` to use `getReplacedSegments`? In that way, we can keep the core logic in a single place.
   
   ```
   Set<String> replacedSegments = getReplacedSegments()
   segments.removeAll(replacedSegments)
   ```



##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -808,7 +808,9 @@ public String getTableAggregateMetadata(
       @ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
       @ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
       @ApiParam(value = "Columns name", allowMultiple = true) @QueryParam("columns") @DefaultValue("")
-          List<String> columns) {
+          List<String> columns,
+      @ApiParam(value = "Whether to exclude replaced segments in the response") @QueryParam("excludeReplacedSegments")
+      @DefaultValue("false") String excludeReplacedSegments) {

Review Comment:
   Can you check other APIs to see if we can directly read the parameter as `boolean`? I think that we supported that.



##########
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);

Review Comment:
   Can we filter out the segments based on the lineage during the aggregation phase in the controller api logic? I think that your code change will become much much simpler. 
   
   Currently, you are pushing down the filtering all the way to the server side and we are even accessing the lineage from the server side. I don't think that it's required based on the motivation from https://github.com/apache/pinot/issues/10244.



-- 
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