Skip to content
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

kvserver: increase rebalance action impact req #98889

Merged
merged 1 commit into from
Jun 7, 2023

Conversation

kvoli
Copy link
Collaborator

@kvoli kvoli commented Mar 17, 2023

Previously, the store rebalancer would attempt to transfer leases or relocate replicas of a range so long as the load of the range (estimated using the local leaseholder replica) was greater than 0.1% of the store's load.

This commit updates the value to be higher for both lease rebalancing and range rebalancing, specifically:

lease rebalancing: 0.1% -> 0.5%
range rebalancing: 0.1% -> 2.0%

The documented rationale, as for why these values exist is also the reason for the increase: decrease unnecessary churn. Only the hottest ranges are considered, when the impact of a rebalance action for these ranges is estimated to be less than a small fraction of the store's total load, it is indicative that the distribution of replica load is not skewed. Letting the replicate queue balance replica and lease counts is a better option in such distributions (until we consolidate rebalancing responsibility).

Making the minimum for range rebalancing higher is also added, as multiple replicas for a range is inherently more costly than moving a lease.

Resolves: #98890

Release note: None

@blathers-crl
Copy link

blathers-crl bot commented Mar 17, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@kvoli kvoli force-pushed the 230317.sr-min-fraction branch from eda0b63 to c9808eb Compare March 17, 2023 20:24
@kvoli kvoli self-assigned this Mar 17, 2023
@kvoli kvoli force-pushed the 230317.sr-min-fraction branch from c9808eb to 8a82539 Compare March 17, 2023 21:15
Previously, the store rebalancer would attempt to transfer leases or
relocate replicas of a range so long as the load of the range (estimated
using the local leaseholder replica) was greater than 0.1% of the
store's load.

This commit updates the value to be higher for both lease rebalancing
and range rebalancing, specifically:

lease rebalancing: 0.1% -> 0.5%
range rebalancing: 0.1% -> 2.0%

The documented rationale, as for why these values exist is also the
reason for the increase: decrease unnecessary churn. Only the hottest
ranges are considered, when the impact of a rebalance action for these
ranges is estimated to be less than a small fraction of the store's
total load, it is indicative that the distribution of replica load is
not skewed. Letting the replicate queue balance replica and lease counts
is a better option in such distributions.

Making the minimum for range rebalancing higher is also added, as
multiple replicas for a range is inherently more costly than moving a
lease.

Resolves: cockroachdb#98890

Release note: None
@kvoli kvoli force-pushed the 230317.sr-min-fraction branch from 8a82539 to a944e57 Compare March 20, 2023 12:48
@kvoli kvoli requested a review from andrewbaptist March 20, 2023 13:33
@kvoli kvoli marked this pull request as ready for review March 20, 2023 13:38
@kvoli kvoli requested a review from a team as a code owner March 20, 2023 13:38
Copy link
Collaborator

@andrewbaptist andrewbaptist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: I'm a little concerned with the potential impact of this change in the sense that it makes some rebalancing less likely to occur, but as you mentioned, the replicate_queue will also do rebalancing. I'm not sure we should backport this as I'm not aware of any field cases that are impacted negativity by this.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @kvoli)

@kvoli
Copy link
Collaborator Author

kvoli commented Apr 18, 2023

Won't backport. I agree, it's generally a risky change and we don't have a pressing need.

@kvoli
Copy link
Collaborator Author

kvoli commented Jun 7, 2023

In light of #104292 I'm going to merge this now and see how we do.

@kvoli
Copy link
Collaborator Author

kvoli commented Jun 7, 2023

bors r=andrewbaptist

@craig
Copy link
Contributor

craig bot commented Jun 7, 2023

Build failed:

@kvoli
Copy link
Collaborator Author

kvoli commented Jun 7, 2023

bors r=andrewbaptist

@craig
Copy link
Contributor

craig bot commented Jun 7, 2023

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kvserver: increase min action impact required
3 participants