xiangfu0 commented on code in PR #19878:
URL: https://github.com/apache/pinot/pull/19878#discussion_r4289839126
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/ReplicaGroupInstanceSelector.java:
##########
@@ -266,7 +266,8 @@ void updateSegmentMapsForUpsertTable(IdealState idealState,
ExternalView externa
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.
Review Comment:
This reasoning is correct: the candidate cache is keyed per
InstanceStateMaps (equal IS+EV maps), and the unavailable-instances map is
keyed by idealStateInstanceStateMap.keySet(), so segments sharing one
InstanceStateMaps necessarily resolve to the same unavailable set. Good to have
it written down.
##########
pinot-broker/src/test/java/org/apache/pinot/broker/routing/instanceselector/InstanceSelectorTest.java:
##########
@@ -2436,8 +2436,19 @@ public void testSortByKeyIfNeeded() {
/// 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
+ {REPLICA_GROUP_INSTANCE_SELECTOR_TYPE, true}
Review Comment:
Nice that the upsert case here actually routes through
updateSegmentMapsForUpsertTable (via isUpsertEnabled), which was the untested
sharing path. Worth noting you intentionally skip an upsert variant for STRICT,
since that selector is already strict regardless of the flag.
--
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]
