gortiz commented on code in PR #19785:
URL: https://github.com/apache/pinot/pull/19785#discussion_r4280603302
##########
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:
Good point. I will add a short comment in a follow-up PR.
##########
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:
Right, the key set is the same for both segments, so the unavailable
instances are the same. I will add this to the comment in the follow-up PR.
##########
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:
Agreed. I will add the upsert variant of this test in the follow-up 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]
