You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2026/10/15 08:46:09 UTC
[PR] Document candidate list sharing invariants and test them for upsert tables [pinot]
gortiz opened a new pull request, #19878:
URL: https://github.com/apache/pinot/pull/19878
## Summary
Follow-up to #19785, part of #19782. This PR applies the review suggestions from #19785. It adds two comments and one test case. It does not change behavior.
## Changes
- `BaseInstanceSelector.InstanceStateMaps.equals()` compares the instance state maps by content, so maps with the same entries in a different iteration order share one canonical instance. A comment now states why this is correct: every candidate list is built in the order of `sortByKeyIfNeeded()`, which does not depend on the iteration order of the maps.
- `ReplicaGroupInstanceSelector.updateSegmentMapsForUpsertTable()` caches the old segment candidates per `InstanceStateMaps`. The candidates also depend on the unavailable instances, which are keyed by the instances of the ideal state. The comment now states that segments that share one `InstanceStateMaps` have equal ideal state maps, thus the same unavailable instances.
- `InstanceSelectorTest.testSharedCandidateListsMatchPerSegmentLists` also runs the replica-group selector on an upsert table. That selector then applies the strict replica-group rules through `updateSegmentMapsForUpsertTable()`. The strict replica-group selector already used this path in the test.
## Tests
`InstanceSelectorTest`, `ReplicaGroupSelectorTest` and `MultiStageReplicaGroupSelectorTest` run 117 tests with 0 failures.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Document candidate list sharing invariants and test them for upsert tables [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19878:
URL: https://github.com/apache/pinot/pull/19878#issuecomment-6189238181
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19878?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 58.65%. Comparing base ([`ba49142`](https://app.codecov.io/gh/apache/pinot/commit/ba49142265f93e4eef17c42c6dc262ab9c9e9f5b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5162f9e`](https://app.codecov.io/gh/apache/pinot/commit/5162f9e15b7961da2d93f5ef16de11b02aeadbc6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19878 +/- ##
=============================================
+ Coverage 40.93% 58.65% +17.71%
+ Complexity 1490 1 -1489
=============================================
Files 3538 2723 -815
Lines 232473 170342 -62131
Branches 37135 28008 -9127
=============================================
+ Hits 95171 99916 +4745
+ Misses 128825 61970 -66855
+ Partials 8477 8456 -21
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19878/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.65% <ø> (+17.71%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.65% <ø> (+17.71%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.65% <ø> (+17.72%)` | :arrow_up: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.65% <ø> (?)` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19878?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Document candidate list sharing invariants and test them for upsert tables [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Document candidate list sharing invariants and test them for upsert tables [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19878:
URL: https://github.com/apache/pinot/pull/19878#discussion_r4289838247
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -650,6 +650,9 @@ public boolean equals(Object o) {
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 instance. This is correct because every candidate list is built in the order of
Review Comment:
Small wording nit: "share one canonical instance" reads a bit ambiguously here, since "instance" is also the domain term for a server node. Maybe say "share one canonical InstanceStateMaps" to make it clear you mean the cached object, not a server.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Document candidate list sharing invariants and test them for upsert tables [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #19878:
URL: https://github.com/apache/pinot/pull/19878#discussion_r4293714705
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -650,6 +650,9 @@ public boolean equals(Object o) {
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 instance. This is correct because every candidate list is built in the order of
Review Comment:
Good point, changed it to 'one canonical InstanceStateMaps'.
##########
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:
Added a note in the data provider: the strict replica-group selector applies these rules for every table, so it needs no upsert case.
##########
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:
Thanks!
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Document candidate list sharing invariants and test them for upsert tables [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #19878:
URL: https://github.com/apache/pinot/pull/19878
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org