gortiz commented on code in PR #19785:
URL: https://github.com/apache/pinot/pull/19785#discussion_r4256310760
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -559,4 +613,81 @@ int getPool(String instanceID) {
/// servers and let servers decide how to handle them.
protected abstract InstanceMapping select(List<String> segments, int
requestId, SegmentStates segmentStates,
Map<String, String> queryOptions);
+
+ /// The ideal state and external view instance state maps of a segment,
compared by content. Within one rebuild,
+ /// segments with equal maps get the same routing candidates, so the
canonical instance (see
+ /// [InstanceStateMapsCache]) caches what is derived from the maps, and the
segments share it.
+ ///
+ /// Not thread safe. Only used by the rebuild thread, for the duration of
one rebuild.
+ static final class InstanceStateMaps {
+ final Map<String, String> _idealStateInstanceStateMap;
+ @Nullable
+ final Map<String, String> _externalViewInstanceStateMap;
+ private final int _hashCode;
+ // Derived from the maps, computed on first use
+ @Nullable
+ Set<String> _onlineInstances;
+ @Nullable
+ List<SegmentInstanceCandidate> _oldSegmentCandidates;
+ // Used by the strict replica-group selector to collect the unavailable
instances once per instance state maps
+ boolean _unavailableInstancesCollected;
+
+ InstanceStateMaps(Map<String, String> idealStateInstanceStateMap,
+ @Nullable Map<String, String> externalViewInstanceStateMap) {
+ _idealStateInstanceStateMap = idealStateInstanceStateMap;
+ _externalViewInstanceStateMap = externalViewInstanceStateMap;
+ _hashCode = 31 * idealStateInstanceStateMap.hashCode() +
Objects.hashCode(externalViewInstanceStateMap);
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (!(o instanceof InstanceStateMaps)) {
+ return false;
+ }
+ InstanceStateMaps that = (InstanceStateMaps) o;
+ return _hashCode == that._hashCode &&
_idealStateInstanceStateMap.equals(that._idealStateInstanceStateMap)
+ && Objects.equals(_externalViewInstanceStateMap,
that._externalViewInstanceStateMap);
+ }
+
+ @Override
+ public int hashCode() {
+ return _hashCode;
+ }
+ }
+
+ /// Returns one canonical [InstanceStateMaps] per distinct pair of instance
state maps, so that the segments of one
+ /// rebuild with equal maps share what is derived from them.
+ ///
+ /// The cache keeps at most `maxSize` pairs. Past that bound, it returns a
new [InstanceStateMaps] for each call,
+ /// which makes each segment compute its own state, as if there was no
sharing.
+ ///
+ /// Not thread safe. Only used by the rebuild thread, for the duration of
one rebuild.
+ static final class InstanceStateMapsCache {
+ private final int _maxSize;
+ private final Map<InstanceStateMaps, InstanceStateMaps> _cache = new
HashMap<>();
+
+ InstanceStateMapsCache(int maxSize) {
+ _maxSize = maxSize;
+ }
+
+ InstanceStateMaps canonicalize(Map<String, String>
idealStateInstanceStateMap,
+ @Nullable Map<String, String> externalViewInstanceStateMap) {
+ InstanceStateMaps instanceStateMaps =
+ new InstanceStateMaps(idealStateInstanceStateMap,
externalViewInstanceStateMap);
+ if (_maxSize <= 0) {
+ return instanceStateMaps;
+ }
+ InstanceStateMaps canonical = _cache.get(instanceStateMaps);
Review Comment:
Thanks, I ran it. It's a synthetic realtime table with 380k segments, 200
servers and 3 replicas, measuring one `onAssignmentChange` against the real
pre-PR revision (43ecf19), with 64 to 380k distinct pairs. The PR allocates
less and is as fast or faster in every case, including past the 4096 limit and
with all pairs distinct. Removing the per-segment TreeMap/TreeSet and the list
copies saves more than the wrapper and hashing cost. Allocation grows a bit
after the cache saturates, so I kept the lookups as you suggested.
Balanced:
| Distinct pairs | Before | After |
|---|---|---|
| 64 | 234 MB / 108 ms | 40 MB / 97 ms |
| 4096 | 234 MB / 309 ms | 41 MB / 255 ms |
| 8192 | 234 MB / 287 ms | 68 MB / 271 ms |
| 50k | 234 MB / 323 ms | 91 MB / 241 ms |
| 380k | 234 MB / 320 ms | 95 MB / 93 ms |
Strict replica group (upsert):
| Distinct pairs | Before | After |
|---|---|---|
| 64 | 259 MB / 505 ms | 53 MB / 262 ms |
| 4096 | 332 MB / 595 ms | 56 MB / 297 ms |
| 8192 | 394 MB / 750 ms | 159 MB / 460 ms |
| 50k | 896 MB / 2107 ms | 654 MB / 1455 ms |
| 380k | 6778 MB / 18.5 s | 6299 MB / 16.2 s |
One side note: strict replica group with upsert scales badly with the number
of distinct pairs on master too, reaching 6.8 GB and 18 s at 380k. Real strict
tables have partition-aligned assignments, so this is an extreme case. It still
needs a follow-up, but I won't be able to work on it for now.
--
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]
