kvserver: version gate locality-aware load-based rebalancing#80190
Conversation
|
This change is |
6c95ef6 to
1439337
Compare
1439337 to
c2dc4bb
Compare
nvb
left a comment
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15)
-- commits, line 7 at r1:
Could you link to the commit(s) that this is referencing? I know a lot of this was changed in 13ac983, but it's difficult for me to convince myself that the logic in pkg/kv/kvserver/deprecated_store_rebalancer.go is exactly the same as it was before that commit. It doesn't appear to be. Were there other changes in this area that impacted that logic?
Did you pull pkg/kv/kvserver/deprecated_store_rebalancer.go straight from the release-21.2 release branch? Were any changes needed?
pkg/cmd/roachtest/tests/rebalance_load.go, line 72 at r2 (raw file):
// Upgrade some (or all) of the first N-1 nodes. Ignore the very last node // since it is the app node. lastNodeToUpgrade := rand.Intn(c.Spec().NodeCount - 1)
rand.Intn returns a value from the interval [0,n). So this is returning a value between [0, node_count - 1). We then pass this to c.Range, which expects 1-indexed node IDs. Does this mean that we'll never update the last CRDB node and sometimes not even update the first?
pkg/kv/kvserver/allocator_scorer.go, line 919 at r1 (raw file):
// NB: This is only applicable in mixed version clusters. // `rankedCandidateListForAllocation` will never be called in 22.1 with a // `qpsScorerOptions`.
When should we plan to remove this kind of compatibility logic?
c2dc4bb to
1a9d18d
Compare
There was a problem hiding this comment.
Reviewable status:
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Could you link to the commit(s) that this is referencing? I know a lot of this was changed in 13ac983, but it's difficult for me to convince myself that the logic in
pkg/kv/kvserver/deprecated_store_rebalancer.gois exactly the same as it was before that commit. It doesn't appear to be. Were there other changes in this area that impacted that logic?Did you pull
pkg/kv/kvserver/deprecated_store_rebalancer.gostraight from therelease-21.2release branch? Were any changes needed?
This was pulled straight from release-21.2 with no material changes. None of the tests were changed either.
I did so because, as you mentioned, there were a bunch of changes in the area because of #65379 and #72296 (and other minor commits), which I didn't want.
pkg/cmd/roachtest/tests/rebalance_load.go, line 72 at r2 (raw file):
Does this mean that we'll never update the last CRDB node
Yep, this was intentional, though I did want to update at least one node. Updated.
pkg/kv/kvserver/allocator_scorer.go, line 919 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
When should we plan to remove this kind of compatibility logic?
Updated the comment.
We don't need to merge this PR on master but I thought that was a good idea regardless, in order to get the extra CI coverage.
01d15a6 to
0bdf58f
Compare
nvb
left a comment
There was a problem hiding this comment.
Reviewed 4 of 5 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status:
2809e19 to
a93168a
Compare
nvb
left a comment
There was a problem hiding this comment.
Reviewed 10 of 13 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status:
This commit introduces a set of deprecated store rebalancer methods corresponding to the pre-22.1 load-rebalancing scheme. Until a store detects that the version upgrade (to 22.1) has been finalized, the store will fall back to the old (pre-22.1) load-based rebalancing logic that wasn't locality aware. This is done in order to minimize risk of unexpected behavior in mixed version clusters. Resolves cockroachdb#76702 Release note: None
…htests This is meant to de-risk the first commit in this patch, which re-introduces the 21.2 store-rebalancer logic to be used in mixed-version settings. Release note: None
a93168a to
d819235
Compare
|
TFTR! bors r+ |
|
Bumping the priority on this so this can go in before I merge the backport and we can close the blocker out. bors p=999 |
|
Build succeeded: |
This code was (re-)added in cockroachdb#80190 to version gate the new 22.1 store rebalancer behavior by keeping the 21.2 one form around. In 22.2 code though we can and did get rid of the version gate (cockroachdb#85777), so all this code can be removed. Release note: None
86060: kvserver: rm EnableNewStoreRebalancer detritus r=irfansharif a=irfansharif This code was (re-)added in #80190 to version gate the new 22.1 store rebalancer behavior by keeping the 21.2 form around. In 22.2 code though we can and did get rid of the version gate (#85777), so all this code can be removed. Release note: None 86064: vendor: bump Pebble to 84fa6a019ed4 r=erikgrinaker,nicktrav a=jbowens ``` 84fa6a01 db: add Metrics.Keys.RangeKeySetsCount f4b082db db: avoid defragmenting beyond prefix bounds during prefix iteration c135b6df metamorphic: use pointer receiver ``` Release note: None Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com> Co-authored-by: Jackson Owens <jackson@cockroachlabs.com>
This commit introduces a set of deprecated store rebalancer methods
corresponding to the pre-22.1 load-rebalancing scheme. Until a store detects
that the version upgrade (to 22.1) has been finalized, the store will fall back
to the old (pre-22.1) load-based rebalancing logic that wasn't locality aware.
This is done in order to minimize risk of unexpected behavior in mixed version
clusters.
All the tests corresponding to the old logic have been re-introduced, and are unchanged.
Resolves #76702
Release note: None