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