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: gossip store read amplification #77040

Merged
merged 1 commit into from
Feb 27, 2022

Conversation

kvoli
Copy link
Collaborator

@kvoli kvoli commented Feb 25, 2022

Previously, other store statistics were gossipped between stores to
inform allocation and rebalancing decisions. This patch introduces an
additional gosipped value, the store read amplification. This patch
allows the allocator to avoid rebalancing ranges onto stores with poor
lsm health, indicated by a large read amplification.

resolves #74254

Release note: None

@kvoli kvoli self-assigned this Feb 25, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@kvoli kvoli force-pushed the gossip-readamp branch 4 times, most recently from 7683898 to 1008d5c Compare February 25, 2022 16:54
@kvoli kvoli requested a review from aayushshah15 February 25, 2022 16:55
@kvoli kvoli marked this pull request as ready for review February 25, 2022 16:55
@kvoli kvoli requested a review from a team as a code owner February 25, 2022 16:55
Copy link
Contributor

@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.

:lgtm:

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


pkg/roachpb/metadata.proto, line 334 at r1 (raw file):

  optional double writes_per_second = 5 [(gogoproto.nullable) = false];
  // read_amplification tracks the current read amplification in the store.
  optional int64 read_amplification = 11 [(gogoproto.nullable) = false];

We should add a TODO here or somewhere else that mentions that the use of this field will need to be version-gated.

@kvoli
Copy link
Collaborator Author

kvoli commented Feb 26, 2022

Thanks for the review.

  optional double writes_per_second = 5 [(gogoproto.nullable) = false];
  // read_amplification tracks the current read amplification in the store.
  optional int64 read_amplification = 11 [(gogoproto.nullable) = false];

We should add a TODO here or somewhere else that mentions that the use of this field will need to be version-gated.

That sounds like a good idea. What would be the process here with mixed versions clusters? Does version gating ensure no overlapping of mismatched versions occurs?

@aayushshah15
Copy link
Contributor

What would be the process here with mixed versions clusters?

When we extend the allocator logic to use this new attribute, it will need to ensure that the current cluster version is one that supports this new ReadAmplication attribute (in other words, it will need to ensure that there are no older nodes in the cluster that aren't broadcasting this new attribute). For a random example, see here:

if !repl.store.cfg.Settings.Version.IsActive(ctx,
clusterversion.EnableLeaseHolderRemoval) {

If this check fails, that means we're in a mixed version state and we can't use this new attribute.

@kvoli
Copy link
Collaborator Author

kvoli commented Feb 26, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 27, 2022

Build failed:

Previously, other store statistics were gossipped between stores to
inform allocation and rebalancing decisions. This patch introduces an
additional gosipped value, the store read amplification. This patch
allows the allocator to avoid rebalancing ranges onto stores with poor
lsm health, indicated by a large read amplification.

resolves cockroachdb#74254

Release note: None
@kvoli
Copy link
Collaborator Author

kvoli commented Feb 27, 2022

bors r-

@kvoli
Copy link
Collaborator Author

kvoli commented Feb 27, 2022

Build failed:

* [GitHub CI (Cockroach)](https://teamcity.cockroachdb.com/viewLog.html?buildId=4467367&buildTypeId=Cockroach_UnitTests)

Rebased on master, going to re-run the CI here.

@kvoli
Copy link
Collaborator Author

kvoli commented Feb 27, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 27, 2022

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, gossip: broadcast read amplification levels in StoreCapacity
3 participants