xiangfu0 commented on code in PR #19785:
URL: https://github.com/apache/pinot/pull/19785#discussion_r4278271001
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -559,4 +615,82 @@ 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) {
Review Comment:
Nice subtlety worth a one-line comment here: `equals` compares the maps by
content (order-independent `Map.equals`), so two segments whose maps are equal
but iterate in different orders canonicalize to the same entry and share one
list. That stays correct only because the candidate list is always built
through `sortByKeyIfNeeded`, which gives a key-sorted order regardless of
iteration order. A short note tying the by-content equality to the sort would
protect this invariant from a future change that drops the sort.
##########
pinot-broker/src/test/java/org/apache/pinot/broker/routing/instanceselector/InstanceSelectorTest.java:
##########
@@ -2406,4 +2410,189 @@ public void
testSelectedPoolMetricsAndRequestIsolation(String selectorType) {
assertEquals(first.getSegmentToInstanceMap(), expectedInstances);
assertEquals(first.getOptionalSegmentToInstanceMap(), Map.of("optional",
"instance128"));
}
+
+ @Test
+ public void testSortByKeyIfNeeded() {
+ Map<String, String> sorted = new LinkedHashMap<>();
+ sorted.put("a", ONLINE);
+ sorted.put("b", ONLINE);
+ sorted.put("c", ONLINE);
+ assertSame(BaseInstanceSelector.sortByKeyIfNeeded(sorted), sorted);
+
+ Map<String, String> treeMap = new TreeMap<>(sorted);
+ assertSame(BaseInstanceSelector.sortByKeyIfNeeded(treeMap), treeMap);
+
+ Map<String, String> unsorted = new LinkedHashMap<>();
+ unsorted.put("c", ONLINE);
+ unsorted.put("a", ONLINE);
+ unsorted.put("b", ONLINE);
+ Map<String, String> sortedCopy =
BaseInstanceSelector.sortByKeyIfNeeded(unsorted);
+ assertNotSame(sortedCopy, unsorted);
+ assertEquals(new ArrayList<>(sortedCopy.keySet()), List.of("a", "b", "c"));
+ assertEquals(sortedCopy, unsorted);
+ }
+
+ /// Segments with equal ideal state and external view instance state maps
share one candidate list within a
+ /// rebuild. Checks that the sharing does not change any routing outcome, by
comparing a selector that shares with
+ /// one that builds a list per segment, and that the candidates keep the
sorted instance order when the instance
+ /// state maps are not in sorted order.
+ @Test(dataProvider = "selectorType")
+ public void testSharedCandidateListsMatchPerSegmentLists(String
selectorType) {
Review Comment:
Great test. One coverage gap: it only drives non-upsert tables
(`createTestInstanceSelector` builds a non-upsert config), so the
`updateSegmentMapsForUpsertTable` path — where the new
`_unavailableInstancesCollected` flag and the
`_onlineInstances`/`_oldSegmentCandidates` caching interleave with the
unavailable-instances aggregation — isn't exercised by this
shared-vs-per-segment comparison. Existing upsert tests guard the refactor, but
adding an upsert case to this comparison would directly verify the sharing
invariant on its most intricate path.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/ReplicaGroupInstanceSelector.java:
##########
@@ -252,30 +261,36 @@ void updateSegmentMapsForUpsertTable(IdealState
idealState, ExternalView externa
}
// Iterate over the maps and exclude the unavailable instances
- for (Map.Entry<String, Set<String>> entry :
oldSegmentToOnlineInstancesMap.entrySet()) {
+ for (Map.Entry<String, InstanceStateMaps> entry :
oldSegmentToInstanceStateMaps.entrySet()) {
String segment = entry.getKey();
- // NOTE: onlineInstances is either a TreeSet or an EmptySet (sorted)
- Set<String> onlineInstances = entry.getValue();
+ InstanceStateMaps instanceStateMaps = entry.getValue();
Map<String, String> idealStateInstanceStateMap =
idealStateAssignment.get(segment);
- Set<String> unavailableInstances =
unavailableInstancesMap.get(idealStateInstanceStateMap.keySet());
- List<SegmentInstanceCandidate> candidates = new
ArrayList<>(onlineInstances.size());
- int idealStateReplicaId = 0;
- for (String instance :
convertToSortedMap(idealStateInstanceStateMap).keySet()) {
- if (onlineInstances.contains(instance) &&
!unavailableInstances.contains(instance)) {
- candidates.add(new SegmentInstanceCandidate(instance, true,
getPool(instance), idealStateReplicaId));
+ // The candidates only depend on the instance state maps: the
unavailable instances are a function of the
+ // instances in the ideal state, and they are final once computed above
+ List<SegmentInstanceCandidate> candidates =
instanceStateMaps._oldSegmentCandidates;
Review Comment:
Worth confirming in the comment why caching the old-segment candidates here
is safe: the candidates depend on `unavailableInstances`, which is keyed by the
ideal-state key set, but two segments sharing this `InstanceStateMaps` always
share the same key set, so they always get the same `unavailableInstances`. The
existing comment above explains the online-instances part; a word on the
unavailable-instances part would make the caching obviously sound.
--
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]
