storage: Avoid adding all replicas at once in RelocateRange #29684
Conversation
|
This change is |
There was a problem hiding this comment.
This basically LGTM. Let's make it an admin request and call it a day! 🎉 🎉 🎉
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained
a-robinson
force-pushed
the
store-rebalance2
branch
from
2e09efa to
434c220
Compare
|
This is ready for final review now, @benesch. Thanks in advance! |
a-robinson
changed the title
There was a problem hiding this comment.
Awesome!
Reviewed 14 of 14 files at r2.
Reviewable status:
a-robinson
force-pushed
the
store-rebalance2
branch
from
434c220 to
6cf3e51
Compare
|
Ugh, a sql logictest is now getting really finicky because the order of the replicas as specified in the |
a-robinson
force-pushed
the
store-rebalance2
branch
from
6cf3e51 to
91fbe19
Compare
|
@jordanlewis can you take a quick look at the new first commit? Commit message copied here for convenience: sql: Sort replicas in crdb_internal.ranges table by store ID This matches the behavior of Starting replicas: [s4] Release note (sql change): The entries in the replicas array column of |
a-robinson
force-pushed
the
store-rebalance2
branch
from
91fbe19 to
15e92df
Compare
This matches the behavior of `SHOW RANGES`, and is the only way for the replicas to be meaningfully ordered in a consistent way. It's odd that the order doesn't even match the order in which the replicas are added. For example, I could see this sequence in the logs for a range: Starting replicas: [s4] Adding s1 results in: [s4,s1] Adding s2 results in: [s4,s1,s2] Removing s4 results in: [s2,s1] Release note (sql change): The entries in the replicas array column of the crdb_internal.ranges virtual table are now always sorted by store ID.
a-robinson
force-pushed
the
store-rebalance2
branch
2 times, most recently
from
7692de0 to
7c66418
Compare
And convert it to an Admin command, AdminRelocateRange, as was required to both let RelocateRange access the local store's resources while still making it callable from the sql package. It now does a more gradual process of adding one replica then removing one until all the desired changes have been made, using the existing allocator code to do so in a reasonable order. Fixes cockroachdb#29130 Release note (sql change): The EXPERIMENTAL_RELOCATE statement no longer temporarily increases the number of replicas in a range more than one above the range's replication factor, preventing rare edge cases of unavailability.
a-robinson
force-pushed
the
store-rebalance2
branch
from
7c66418 to
4b28b0b
Compare
There was a problem hiding this comment.
If you'll accept my review in place of @jordanlewis's: LGTM!
|
bors r+ |
Build failed |
|
That's a new one to me:
bors r+ |
Build failed |
|
Ah, it's from the recent go-test-teamcity update (cockroachdb/go-test-teamcity#9). I'll wait until that's resolved (cockroachdb/go-test-teamcity#10). |
|
Retrying after merging cockroachdb/go-test-teamcity#10 bors r+ |
29684: storage: Avoid adding all replicas at once in RelocateRange r=a-robinson a=a-robinson Does a more gradual process of adding one replica then removing one until all the desired changes have been made, using the existing allocator code to do so in a reasonable order. Fixes #29130 Release note: None ------------------- This is still a work-in-progress in the sense that it isn't really usable from the SQL code yet, since I just implemented it as a method on `Store`. If the approach looks reasonable, though, it can be converted it into an Admin API method. Co-authored-by: Alex Robinson <[email protected]>
Build succeeded |
Does a more gradual process of adding one replica then removing one
until all the desired changes have been made, using the existing
allocator code to do so in a reasonable order.
Fixes #29130
Release note: None
This is still a work-in-progress in the sense that it isn't really usable from the SQL code yet, since I just implemented it as a method on
Store. If the approach looks reasonable, though, it can be converted it into an Admin API method.