BackendArchitectX commented on code in PR #19915:
URL: https://github.com/apache/pinot/pull/19915#discussion_r4416884798
##########
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);
Review Comment:
Thanks for the detailed review, Jackie. I've updated the implementation in
`1b24c35fac` to address the key-allocation concern.
The unavailable-instance map now uses `Object2ObjectOpenCustomHashMap` with
a custom `Hash.Strategy`, reusing the existing assignment `keySet()` views.
This eliminates the per-lookup wrapper, array copy and sort.
I used order-independent mixed hashing with `Set.equals()` rather than
iteration-order equality. This preserves consistent hashing and equality when
assignments contain the same instances in different iteration orders.
Validation:
- 97/97 targeted tests passed.
- Added coverage for reordered sets and assignment/Helix serialization round
trips.
- Hash-distribution regression covers 100,000 combinations.
- Scoped Checkstyle and Spotless passed.
JMH results for 50,000 segments, 200 servers, 3 replicas and 3 JVM forks:
| Implementation | Healthy (ms) | Partial failure (ms) | Healthy alloc.
(MiB) |
|---|---:|---:|---:|
| Original Set-key | 186.64 | 191.32 | 34.91 |
| Sorted InstanceSetKey | 78.58 | 82.72 | 40.35 |
| Custom fastutil strategy | 114.10 | 93.28 | 32.95 |
The custom strategy reduces allocation versus the sorted-key approach, but
is slower in these measurements. Healthy-case results also showed significant
fork-to-fork variation.
The 380,000-segment workload, additional distinct-set counts and GC
measurements remain outstanding, so the production-scale performance trade-off
is not yet established.
I'd appreciate your feedback on this approach and whether further changes
are needed.
--
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]
