This is an automated email from the ASF dual-hosted git repository.
gortiz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 65175a46323 Document candidate list sharing invariants and test them
for upsert tables (#19878)
65175a46323 is described below
commit 65175a46323ce56ca62171827630435feda7e40e
Author: Gonzalo Ortiz Jaureguizar <[email protected]>
AuthorDate: Fri Oct 16 13:55:08 2026 +0200
Document candidate list sharing invariants and test them for upsert tables
(#19878)
Follow-up to #19785, part of #19782.
---
.../routing/instanceselector/BaseInstanceSelector.java | 3 +++
.../instanceselector/ReplicaGroupInstanceSelector.java | 3 ++-
.../routing/instanceselector/InstanceSelectorTest.java | 18 +++++++++++++++---
3 files changed, 20 insertions(+), 4 deletions(-)
diff --git
a/pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java
b/pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java
index 263dc258457..cedb83d57ff 100644
---
a/pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java
+++
b/pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java
@@ -650,6 +650,9 @@ public abstract class BaseInstanceSelector implements
InstanceSelector {
return false;
}
InstanceStateMaps that = (InstanceStateMaps) o;
+ // NOTE: Map.equals() ignores the iteration order, so maps with the same
entries in a different order share one
+ // canonical InstanceStateMaps. This is correct because every
candidate list is built in the order of
+ // sortByKeyIfNeeded(), which does not depend on the iteration
order of the maps.
return _hashCode == that._hashCode &&
_idealStateInstanceStateMap.equals(that._idealStateInstanceStateMap)
&& Objects.equals(_externalViewInstanceStateMap,
that._externalViewInstanceStateMap);
}
diff --git
a/pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/ReplicaGroupInstanceSelector.java
b/pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/ReplicaGroupInstanceSelector.java
index 9f1be381206..e62d99cfb33 100644
---
a/pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/ReplicaGroupInstanceSelector.java
+++
b/pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/ReplicaGroupInstanceSelector.java
@@ -266,7 +266,8 @@ public class ReplicaGroupInstanceSelector extends
BaseInstanceSelector {
InstanceStateMaps instanceStateMaps = entry.getValue();
Map<String, String> idealStateInstanceStateMap =
idealStateAssignment.get(segment);
// 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
+ // instances in the ideal state, and they are final once computed above.
Segments that share one
+ // InstanceStateMaps have equal ideal state maps, thus the same
instances and the same unavailable instances.
List<SegmentInstanceCandidate> candidates =
instanceStateMaps._oldSegmentCandidates;
if (candidates == null) {
Set<String> onlineInstances = instanceStateMaps._onlineInstances;
diff --git
a/pinot-broker/src/test/java/org/apache/pinot/broker/routing/instanceselector/InstanceSelectorTest.java
b/pinot-broker/src/test/java/org/apache/pinot/broker/routing/instanceselector/InstanceSelectorTest.java
index 421f3fa9288..c708805f7f9 100644
---
a/pinot-broker/src/test/java/org/apache/pinot/broker/routing/instanceselector/InstanceSelectorTest.java
+++
b/pinot-broker/src/test/java/org/apache/pinot/broker/routing/instanceselector/InstanceSelectorTest.java
@@ -2436,8 +2436,20 @@ public class InstanceSelectorTest {
/// 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) {
+ @DataProvider(name = "sharedCandidateListsCases")
+ public Object[][] getSharedCandidateListsCases() {
+ return new Object[][]{
+ {REPLICA_GROUP_INSTANCE_SELECTOR_TYPE, false},
{STRICT_REPLICA_GROUP_INSTANCE_SELECTOR_TYPE, false},
+ {BALANCED_INSTANCE_SELECTOR, false},
+ // An upsert table makes the replica-group selector apply the strict
replica-group rules. The strict
+ // replica-group selector needs no upsert case, because it applies
these rules for every table.
+ {REPLICA_GROUP_INSTANCE_SELECTOR_TYPE, true}
+ };
+ }
+
+ @Test(dataProvider = "sharedCandidateListsCases")
+ public void testSharedCandidateListsMatchPerSegmentLists(String
selectorType, boolean upsert) {
+ when(_tableConfig.isUpsertEnabled()).thenReturn(upsert);
// Partition 0: the ideal state lists the instances out of order. One of
its old segments has a replica OFFLINE in
// the external view.
// Partition 1: one of its instances is disabled.
@@ -2509,7 +2521,7 @@ public class InstanceSelectorTest {
// Candidates are in sorted instance order, with the replica id of the
sorted ideal state, whatever the order of
// the instance state maps
- boolean strict =
selectorType.equals(STRICT_REPLICA_GROUP_INSTANCE_SELECTOR_TYPE);
+ boolean strict =
selectorType.equals(STRICT_REPLICA_GROUP_INSTANCE_SELECTOR_TYPE) || upsert;
// Strict replica-group takes instance_b out of service for the whole
partition, because it is OFFLINE for p0_3
assertEquals(describe(sharedStates.getCandidates("p0_0")),
strict ? List.of("instance_a/0/true", "instance_c/2/true")
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
