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: make the StoreRebalancer interval a cluster setting #78962

Conversation

aayushshah15
Copy link
Contributor

@aayushshah15 aayushshah15 commented Mar 29, 2022

Release note (ops change): the kv.allocator.load_based_rebalancing_interval
cluster setting now lets operators choose the interval at which each store in the
cluster will check for load-based lease or replica rebalancing opportunities.

@aayushshah15 aayushshah15 requested a review from a team as a code owner March 29, 2022 16:09
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

Thanks. Do you think we should backport this to 22.1 and other release branches?

// Setting this interval to a very low duration is generally going to be a
// bad idea without any real benefit, so let's disallow that.
const min = 10 * time.Second
if d <= min {
Copy link
Member

Choose a reason for hiding this comment

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

nit: consider making the allowable range inclusive of 10 seconds so that it's easier to set this setting to its minimum value.

Release note (ops change): the `kv.allocator.load_based_rebalancing_interval`
cluster setting now lets operators the interval at which each store in the
cluster will check for load-based lease or replica rebalancing opportunities.
@aayushshah15 aayushshah15 force-pushed the 20220329_makeStoreRebalancerIntervalAClusterSetting branch from 7228a17 to 119f031 Compare March 29, 2022 17:57
Copy link
Contributor Author

@aayushshah15 aayushshah15 left a comment

Choose a reason for hiding this comment

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

Yes, I think it's good to backport this.

bors r+

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


pkg/kv/kvserver/store_rebalancer.go, line 114 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: consider making the allowable range inclusive of 10 seconds so that it's easier to set this setting to its minimum value.

Done.

@craig
Copy link
Contributor

craig bot commented Mar 29, 2022

Build succeeded:

@craig craig bot merged commit 897c2da into cockroachdb:master Mar 29, 2022
@blathers-crl
Copy link

blathers-crl bot commented Mar 29, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 119f031 to blathers/backport-release-21.1-78962: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.1.x failed. See errors above.


error creating merge commit from 119f031 to blathers/backport-release-21.2-78962: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2.x failed. See errors above.


error creating merge commit from 119f031 to blathers/backport-release-22.1-78962: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

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.

3 participants