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: [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]
