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 l0sublevels instead of read amp #78949

Merged
merged 1 commit into from
Mar 29, 2022

Conversation

kvoli
Copy link
Collaborator

@kvoli kvoli commented Mar 29, 2022

Previously read amplification was gossipped among stores to enable
future allocation decisions that would avoid candidates with high read
amplification. L0 Sublevels represents the number of levels with L0 and
is a portion of read amplification. This patch change read amplification
to l0 sublevels, as it is a better indicator of store health.

Release justification: low risk, replace deprecated gossip signal.

Release note: None

@kvoli kvoli self-assigned this Mar 29, 2022
@kvoli kvoli requested a review from a team as a code owner March 29, 2022 13:51
@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.

:lgtm:

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15 and @kvoli)


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

  // This information can be used for rebalancing decisions.
  optional Percentiles bytes_per_replica = 6 [(gogoproto.nullable) = false];
  optional Percentiles writes_per_replica = 7 [(gogoproto.nullable) = false];

Let's mark field number 11 as reserved. reserved 11; at the end.

Previously read amplification was gossipped among stores to enable
future allocation decisions that would avoid candidates with high read
amplification. L0 sublevels represents the number of levels within the
levle 0 and is normally the significant portion of read amplification.
When read amplification is high (>15) it is normally due to a similarly
high count of L0 sublevels. This patch change read amplification to l0
sublevels, as it is a better indicator of store health.

Release justification: low risk, replace deprecated gossip signal.

Release note: None
@kvoli kvoli force-pushed the 220329.gossip-l0sublevels branch from bafd4e8 to 36d49f3 Compare March 29, 2022 14:12
Copy link
Collaborator Author

@kvoli kvoli left a comment

Choose a reason for hiding this comment

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

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


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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Let's mark field number 11 as reserved. reserved 11; at the end.

Done.

Copy link
Collaborator Author

@kvoli kvoli left a comment

Choose a reason for hiding this comment

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

TYFTR Nathan 🤠

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

@kvoli kvoli removed the request for review from aayushshah15 March 29, 2022 14:22
@kvoli
Copy link
Collaborator Author

kvoli commented Mar 29, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 29, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 29, 2022

Build succeeded:

@craig craig bot merged commit b154814 into cockroachdb:master Mar 29, 2022
@tbg
Copy link
Member

tbg commented Mar 30, 2022

Just for my (and I suspect not only my) education, what is the read amp metric actually measuring? Now that I have learned what L0Sublevels is I'm confused about that, because I thought that was read amp.

craig bot pushed a commit that referenced this pull request Mar 31, 2022
78995: release-22.1: kvserver: gossip l0sublevels instead of read amp r=kvoli a=blathers-crl[bot]

Backport 1/1 commits from #78949 on behalf of `@kvoli.`

/cc `@cockroachdb/release`

----

Previously read amplification was gossipped among stores to enable
future allocation decisions that would avoid candidates with high read
amplification. L0 Sublevels represents the number of levels with L0 and
is a portion of read amplification. This patch change read amplification
to l0 sublevels, as it is a better indicator of store health.

Release justification: low risk, replace deprecated gossip signal.

Release note: None

----

Release justification:

Co-authored-by: Austen McClernon <[email protected]>
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.

4 participants