Jackie-Jiang commented on a change in pull request #7243:
URL: https://github.com/apache/pinot/pull/7243#discussion_r688170952
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/PartitionSegmentPruner.java
##########
@@ -163,48 +190,64 @@ public synchronized void refreshSegment(String segment) {
if (filterExpression == null) {
return segments;
}
- Set<String> selectedSegments = new HashSet<>();
- for (String segment : segments) {
- PartitionInfo partitionInfo = _partitionInfoMap.get(segment);
- if (partitionInfo == null || partitionInfo == INVALID_PARTITION_INFO
|| isPartitionMatch(filterExpression,
- partitionInfo)) {
- selectedSegments.add(segment);
- }
- }
- return selectedSegments;
+ return pruneSegments(segments, (partitionInfo, cachedPartitionFunction)
-> isPartitionMatch(filterExpression,
+ partitionInfo, cachedPartitionFunction));
} else {
// PQL
FilterQueryTree filterQueryTree =
RequestUtils.generateFilterQueryTree(brokerRequest);
if (filterQueryTree == null) {
return segments;
}
- Set<String> selectedSegments = new HashSet<>();
- for (String segment : segments) {
+ return pruneSegments(segments, (partitionInfo, cachedPartitionFunction)
-> isPartitionMatch(filterQueryTree, partitionInfo, cachedPartitionFunction));
+ }
+ }
+
+ private SelectedSegments pruneSegments(SelectedSegments segments,
java.util.function.BiFunction<PartitionInfo, CachedPartitionFunction, Boolean>
partitionMatchLambda) {
+ // Some segments may not be refreshed/notified via Zookeeper yet, but
broker may have found it
+ // in this case we need to include the new segments.
+ Set<String> selectedSegments = new HashSet<>();
+ CachedPartitionFunction cachedPartitionFunction = new
CachedPartitionFunction();
+ if (segments.hasHash() &&
segments.getSegmentHash().equals(_allSegments.getSegmentHash())) {
Review comment:
`TimeSegmentPruner` is always in front of `PartitionSegmentPruner` if
configured, so this if statement won't be `true` if any segment is pruned.
It also adds a lot of complexity to the code as we always need to choose
whether to calculate the hash.
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/PartitionSegmentPruner.java
##########
@@ -88,8 +95,19 @@ public void init(ExternalView externalView, IdealState
idealState, Set<String> o
_partitionInfoMap.put(segment, partitionInfo);
}
}
+ rebuildReverseMap();
}
+ private void rebuildReverseMap() {
+ _partitionInfoSegments.clear();
+ Set<String> segmentsFound = new HashSet<>();
+ _partitionInfoMap.forEach((segmentName, partitionInfo) -> {
+ segmentsFound.add(segmentName);
+ Set<String> segments =
_partitionInfoSegments.computeIfAbsent(partitionInfo, (info) -> new
HashSet<>());
Review comment:
This won't be correct if a segment contains more than 1 partitions. We
need to maintain a `Map<PartitionFunction, Map<Integer, Set<String>>>`, where
the inner map can be optimized to a `List<Set<String>>`, and the index of the
list is the partition number.
--
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]
