allocatorimpl: Prioritize non-voters in voter additions#89650
allocatorimpl: Prioritize non-voters in voter additions#89650craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
46491eb to
a9e862d
Compare
kvoli
left a comment
There was a problem hiding this comment.
Great work on this so far. A few things to add in:
(1) Update the score fn to be consistent in ordering values it returns
(2) Add some testing to exercise the example we want this to work in, then a few where there's too great imbalance or other reasons where a non-voter exists for promotion but we don't want to promote it.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @KaiSun314)
pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer.go line 693 at r1 (raw file):
return -(1 + (float64(o.balanceScore-c.balanceScore))/10.0) } if c.hasNonVoter != o.hasNonVoter {
The above scores are strictly ordered. The 10 is out of order with the other scores returned:
...
converges (+-)[2,3]
balance (+-)[1,2]
nonVoter +-10
This is fine for calling Less which is used to sort them and for allocation.
However it does matter for rebalancing, which won't be in this patch here but we should keep consistent to avoid confusion in the future.
We can adjust the other values up and slot this one in below. or just return a value in the range (+-)[0,1).
422afa8 to
cb0e4cd
Compare
KaiSun314
left a comment
There was a problem hiding this comment.
(1) Done - added a bunch of unit 0s
(2) Done - added 3 unit tests:
- One store with non-voter and range count 516 + one store with voter and range count 480 - make sure allocator selects store with the non-voter despite higher range count
- Stores with non-voter and range count > 475 + one store with voter and range count < 475 (resulting in different balance scores) - make sure allocator selects store with the voter due to higher balance score
- One store with non-voter and range count 516 but does not satisfy voter constraint + one store with voter and range count 480 that satisfies voter constraint - make sure allocator selects store with voter that satisfies voter constraint
Reviewable status:
pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer.go line 693 at r1 (raw file):
Previously, kvoli (Austen) wrote…
The above scores are strictly ordered. The 10 is out of order with the other scores returned:
... converges (+-)[2,3] balance (+-)[1,2] nonVoter +-10This is fine for calling
Lesswhich is used to sort them and for allocation.However it does matter for rebalancing, which won't be in this patch here but we should keep consistent to avoid confusion in the future.
We can adjust the other values up and slot this one in below. or just return a value in the range (+-)[0,1).
Done.
ca5dbe8 to
08176bf
Compare
kvoli
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go line 8186 at r2 (raw file):
} func TestNonVoterPrioritizationInVoterAdditions(t *testing.T) {
Could you add the test scenario descriptions that you mentioned, above each test case to make this easier to parse for future readers.
// NB: ... is overfull despite having a nonvoter, we select store X.pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go line 8280 at r2 (raw file):
}, expectedTargetAllocate: roachpb.ReplicationTarget{NodeID: 4, StoreID: 4}, },
Could you add in the test case from the issue.
Perhaps in a TestMultiiRegion... , just so we can cover our cases.
region 1: voter (leaseholder), voter, voter
region 2: non-voter
region 3: non-voter
08176bf to
178631c
Compare
KaiSun314
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go line 8186 at r2 (raw file):
Previously, kvoli (Austen) wrote…
Could you add the test scenario descriptions that you mentioned, above each test case to make this easier to parse for future readers.
// NB: ... is overfull despite having a nonvoter, we select store X.
Done.
pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go line 8280 at r2 (raw file):
Previously, kvoli (Austen) wrote…
Could you add in the test case from the issue.
Perhaps in a TestMultiiRegion... , just so we can cover our cases.
region 1: voter (leaseholder), voter, voter region 2: non-voter region 3: non-voter
Done.
KaiSun314
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/allocator/allocatorimpl/allocator_test.go line 8280 at r2 (raw file):
Previously, KaiSun314 (Kai Sun) wrote…
Done.
In replicate_queue_test.go
kvoli
left a comment
There was a problem hiding this comment.
small nits otherwise great stuff
Reviewed 1 of 3 files at r2, 1 of 3 files at r3, all commit messages.
Reviewable status:
pkg/kv/kvserver/replicate_queue_test.go line 2015 at r3 (raw file):
Knobs: base.TestingKnobs{ SpanConfig: &spanconfig.TestingKnobs{ ConfigureScratchRange: true,
nit: I don't think you need this field anymore.
pkg/kv/kvserver/replicate_queue_test.go line 2033 at r3 (raw file):
// predictable behaviour when the allocator ranks stores using balance score. _, err := db.Exec( "ALTER DATABASE system CONFIGURE ZONE USING num_replicas = 7, num_voters = 7",
nit: Add a local function
initConstraintFn := func(count int, range string) { ... }
initConstraintFn(7, "system")
...
initConstraintFn(7, "default")0d9f478 to
35a46a9
Compare
KaiSun314
left a comment
There was a problem hiding this comment.
Thank you for the review Austen!
Reviewable status:
|
bors r=kvoli |
|
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
|
GitHub status checks took too long to complete, so bors is giving up. You can adjust bors configuration to have it wait longer if you like. |
Previously, when adding a voter via the allocator, we would not prioritize stores with non-voters when selecting candidate stores. This was inadequate because selecting a store without a non-voter would require sending a snapshot over the WAN in order to add a voter onto that store. On the other hand, selecting a store with a non-voter would only require promoting that non-voter to a voter, no snapshot needs to be sent over the WAN. To address this, this patch determines if candidate stores have a non-voter replica and prioritize this status when sorting candidate stores (priority lower than balance score, but higher than range count). By prioritizing selecting a store with a non-voter, we can reduce the number of snapshots that need to be sent over the WAN. Release note (ops change): We prioritized non-voters in voter additions, meaning that when selecting a store to add a voter on (in the allocator), we would prioritize candidate stores that contain a non-voter replica higher. This allows us to reduce the number of snapshots that need to be sent over the WAN.
35a46a9 to
c637e64
Compare
|
bors r=kvoli |
|
Build failed: |
|
bors r=kvoli |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build succeeded: |
Fix #63810
Previously, when adding a voter via the allocator, we would not prioritize stores with non-voters when selecting candidate stores.
This was inadequate because selecting a store without a non-voter would require sending a snapshot over the WAN in order to add a voter onto that store. On the other hand, selecting a store with a non-voter would only require promoting that non-voter to a voter, no snapshot needs to be sent over the WAN.
To address this, this patch determines if candidate stores have a non-voter replica and prioritize this status when sorting candidate stores (priority lower than balance score, but higher than range count). By prioritizing selecting a store with a non-voter, we can reduce the number of snapshots that need to be sent over the WAN.
Release note (ops change): We prioritized non-voters in voter additions, meaning that when selecting a store to add a voter on (in the allocator), we would prioritize candidate stores that contain a non-voter replica higher. This allows us to reduce the number of snapshots that need to be sent over the WAN.