jackjlli commented on a change in pull request #7243:
URL: https://github.com/apache/pinot/pull/7243#discussion_r681332135
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/PartitionSegmentPruner.java
##########
@@ -278,5 +287,37 @@ private boolean isPartitionMatch(FilterQueryTree
filterQueryTree, PartitionInfo
_partitionFunction = partitionFunction;
_partitions = partitions;
}
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) return true;
+ if (o == null || getClass() != o.getClass()) return false;
+ PartitionInfo that = (PartitionInfo) o;
+ return Objects.equals(_partitionFunction, that._partitionFunction) &&
+ Objects.equals(_partitions, that._partitions);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(_partitionFunction, _partitions);
+ }
+ }
+
+ /**
+ * This class is created to speed up partition number calls. Sometimes with
lots of segments (like 40k+)
+ * Computing partition values may be CPU intensive, caching the computation
result may help
+ */
+ private static class CachedPartitionFunction {
+ final Map<PartitionFunction, Map<String, Integer>> cache = new HashMap<>();
Review comment:
It would be great to add a brief javadoc on the meaning of the inner
hash map.
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/SegmentPrunerFactory.java
##########
@@ -84,6 +82,8 @@ private SegmentPrunerFactory() {
}
}
}
+ // Always prune out empty segments first
Review comment:
Why move it to the bottom?
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/PartitionSegmentPruner.java
##########
@@ -278,5 +287,37 @@ private boolean isPartitionMatch(FilterQueryTree
filterQueryTree, PartitionInfo
_partitionFunction = partitionFunction;
_partitions = partitions;
}
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) return true;
+ if (o == null || getClass() != o.getClass()) return false;
+ PartitionInfo that = (PartitionInfo) o;
+ return Objects.equals(_partitionFunction, that._partitionFunction) &&
+ Objects.equals(_partitions, that._partitions);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(_partitionFunction, _partitions);
+ }
+ }
+
+ /**
+ * This class is created to speed up partition number calls. Sometimes with
lots of segments (like 40k+)
+ * Computing partition values may be CPU intensive, caching the computation
result may help
+ */
+ private static class CachedPartitionFunction {
+ final Map<PartitionFunction, Map<String, Integer>> cache = new HashMap<>();
+
+ public int getPartition(PartitionFunction func, String value) {
Review comment:
Can you elaborate a bit more on the meaning of the `value` here? Does
that mean all the values will be cached in the hashmap and will never get
cleaned up?
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/PartitionSegmentPruner.java
##########
@@ -61,6 +63,7 @@
private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
private final String _segmentZKMetadataPathPrefix;
private final Map<String, PartitionInfo> _partitionInfoMap = new
ConcurrentHashMap<>();
+ private final Map<PartitionInfo, Set<String>> _partitionInfoSegments = new
ConcurrentHashMap<>();
Review comment:
Oh, nvm. Saw your methods down below.
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/PartitionSegmentPruner.java
##########
@@ -61,6 +63,7 @@
private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
private final String _segmentZKMetadataPathPrefix;
private final Map<String, PartitionInfo> _partitionInfoMap = new
ConcurrentHashMap<>();
+ private final Map<PartitionInfo, Set<String>> _partitionInfoSegments = new
ConcurrentHashMap<>();
Review comment:
You may probably need to override the hashCode and equals methods in
`PartitionInfo` class since the `PartitionInfo` object is used as a key in a
hashmap in your PR.
--
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]
