Skip to content

kvserver: gossip store read amplification#77040

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
kvoli:gossip-readamp
Feb 27, 2022
Merged

kvserver: gossip store read amplification#77040
craig[bot] merged 1 commit intocockroachdb:masterfrom
kvoli:gossip-readamp

Conversation

@kvoli
Copy link
Copy Markdown
Contributor

@kvoli kvoli commented Feb 25, 2022 •
edited
Loading

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
Copy Markdown
Member

cockroach-teamcity commented Feb 25, 2022

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
aayushshah15
aayushshah15 approved these changes Feb 25, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

aayushshah15 commented Feb 26, 2022

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:

cockroach/pkg/kv/kvserver/replicate_queue.go

Lines 554 to 555 in 208e2b4

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
Copy Markdown
Contributor Author

kvoli commented Feb 26, 2022

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 27, 2022

Build failed:

@kvoli
kvserver: gossip store read amplification
ffdc631 
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
Copy Markdown
Contributor Author

kvoli commented Feb 27, 2022

bors r-

@kvoli
Copy link
Copy Markdown
Contributor 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
Copy Markdown
Contributor Author

kvoli commented Feb 27, 2022

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 27, 2022

Build succeeded:

@craig craig bot merged commit eee8ef0 into cockroachdb:master Feb 27, 2022
@erikgrinaker erikgrinaker mentioned this pull request Feb 28, 2022
Closed
@kvoli kvoli mentioned this pull request Mar 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@aayushshah15 aayushshah15 aayushshah15 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kvoli kvoli

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

kvserver, gossip: broadcast read amplification levels in StoreCapacity

3 participants

@kvoli @cockroach-teamcity @aayushshah15