-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
storage: Transfer lease to least-loaded store when rebalancing replicas #30256
Conversation
LGTM.
Any way to add a test to make sure the logic is correct?
…On Sat, Sep 15, 2018, 15:44 cockroach-teamcity ***@***.***> wrote:
This change is [image: Reviewable]
<https://reviewable.io/reviews/cockroachdb/cockroach/30256>
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#30256 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABihuVf60jYH7RxX4mFv_ScigUoINyBdks5ubViAgaJpZM4WqgvQ>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, absolutely. Done (along with a couple tweaks to slightly improve decisions). PTAL
38d1c58
to
aa4319d
Compare
ping @BramGruneir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, 3 of 3 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)
bors r+ |
Build failed (retrying...) |
Build failed (retrying...) |
Build failed |
aa4319d
to
8c325ee
Compare
Updated one line in the test to fix merge skew with #30143. bors r+ |
Build failed |
Heh, now that I've rebased on master it's failing due to #30435. I'll fix that up before continuing with this. |
I'll wait on this until #30422 is in, since it fixes the test failures. |
This logic was totally busted before due to indexing into the wrong replicas slice, meaning we would often transfer the lease to the wrong store. Also add a proper test for the replica/lease selection logic. Release note: None
Further protect against picking rebalance targets that are somewhat full by both extending the allocator_scorer convergeScore logic to add an extra score for overfull stores, and add a couple fallback checks to back up the convergeScore logic when needed. Release note: None
8c325ee
to
dda75b7
Compare
bors r+ |
27921: kubernetes: Add a secure config that can use externally created certs r=a-robinson a=a-robinson For Kubernetes clusters that don't sign certificates properly, such as EKS as discovered in a recent forum post: https://forum.cockroachlabs.com/t/secure-cockroachdb-cluster-on-aws-eks/1824 Release note: None -------------------- It's arguable that this isn't worth checking in. I'm not too attached to it if folks don't think we should, I just needed to publish it somewhere for the original poster of https://forum.cockroachlabs.com/t/secure-cockroachdb-cluster-on-aws-eks/1824 to be able to use it. Touches #24527 30256: storage: Transfer lease to least-loaded store when rebalancing replicas r=a-robinson a=a-robinson This logic was totally busted before due to indexing into the wrong replicas slice, meaning we would often transfer the lease to the wrong store. Release note: None Co-authored-by: Alex Robinson <alexdwanerobinson@gmail.com>
Build succeeded |
This logic was totally busted before due to indexing into the wrong
replicas slice, meaning we would often transfer the lease to the wrong
store.
Release note: None