xiangfu0 commented on code in PR #19915:
URL: https://github.com/apache/pinot/pull/19915#discussion_r4344731996
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/ReplicaGroupInstanceSelector.java:
##########
@@ -73,6 +75,45 @@ public class ReplicaGroupInstanceSelector extends
BaseInstanceSelector {
private static final Logger LOGGER =
LoggerFactory.getLogger(ReplicaGroupInstanceSelector.class);
+ // Immutable key for instance groups that reduces hash collisions.
+ static final class InstanceSetKey implements Comparable<InstanceSetKey> {
+ private final String[] _instances;
+ private final int _hashCode;
+
+ InstanceSetKey(Set<String> instances) {
+ _instances = instances.toArray(new String[0]);
+ Arrays.sort(_instances);
+
+ int hash = 1;
+ for (String instance : _instances) {
+ hash = 31 * hash + HashCommon.mix(instance.hashCode());
+ }
+ _hashCode = hash;
+ }
+
+ @Override
+ public int hashCode() {
+ return _hashCode;
+ }
+
+ @Override
+ public boolean equals(Object object) {
+ if (this == object) {
+ return true;
+ }
+ if (!(object instanceof InstanceSetKey)) {
+ return false;
+ }
+ InstanceSetKey other = (InstanceSetKey) object;
+ return _hashCode == other._hashCode && Arrays.equals(_instances,
other._instances);
+ }
+
+ @Override
+ public int compareTo(InstanceSetKey other) {
Review Comment:
Nit: `InstanceSetKey` only ever serves as a `HashMap` key here, never in a
sorted collection, so `Comparable`/`compareTo` is dead code. Fine to drop it to
keep the key minimal (or keep if you expect a `TreeMap` use soon).
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/ReplicaGroupInstanceSelector.java:
##########
@@ -292,8 +336,8 @@ void updateSegmentMapsForUpsertTable(IdealState idealState,
ExternalView externa
Set<String> onlineInstances = entry.getValue()._onlineInstances;
Map<String, String> idealStateInstanceStateMap =
idealStateAssignment.get(segment);
Map<String, String> sortedIdealStateInstanceStateMap =
sortByKeyIfNeeded(idealStateInstanceStateMap);
- Set<String> unavailableInstances =
-
unavailableInstancesMap.getOrDefault(idealStateInstanceStateMap.keySet(),
Set.of());
+ Set<String> unavailableInstances = unavailableInstancesMap.getOrDefault(
+ new InstanceSetKey(idealStateInstanceStateMap.keySet()), Set.of());
Review Comment:
Minor: this builds a fresh `InstanceSetKey` (array copy + sort) per new
segment, which the old `keySet()` lookup avoided. Arrays are tiny
(replica-group size) so the cost is negligible, just flagging the added
per-segment allocation.
--
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]
