-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: Add a metric for in-progress snapshots #99843
kvserver: Add a metric for in-progress snapshots #99843
Conversation
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. |
cb9433a
to
842d2b8
Compare
842d2b8
to
e0a1366
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with release note for renaming failure/success metrics.
Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andrewbaptist)
-- commits
line 9 at r2:
range.snapshots.delegate.failures
and range.snapshot.delegate.successes
had a release note when they were added (4a43cf6). You should add another release note here mentioning the rename, for docs.
pkg/kv/kvserver/replica_command.go
line 2859 at r2 (raw file):
) if !selfDelegate { r.store.Metrics().DelegateSnapshotInProgress.Dec(1)
nit: Did you consider moving this up above in the same if block as the inc and deferring the dec?
e0a1366
to
bea9903
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR - I added the release note.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @kvoli)
Previously, kvoli (Austen) wrote…
range.snapshots.delegate.failures
andrange.snapshot.delegate.successes
had a release note when they were added (4a43cf6). You should add another release note here mentioning the rename, for docs.
Done
pkg/kv/kvserver/replica_command.go
line 2859 at r2 (raw file):
Previously, kvoli (Austen) wrote…
nit: Did you consider moving this up above in the same if block as the inc and deferring the dec?
I did consider this, but then it complains about a defer within a loop. I'm not sure which is better actually but I left this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andrewbaptist)
bea9903
to
b2831cd
Compare
Fixes: cockroachdb#98242 Knowing how many delegate snapshot requests are currently in-progress will be useful for detecting problems. This change adds a metric for this. It also updates the names of the previous stats to have the prefix `range.snapshots` vs `range.snapshot` to be consistent with other stats. Epic: none Release note (ops change): Adds a new stat range.snapshots.delegate.in-progress and renames two existing stats. They were never part of a release, so better to rename them before 23.1.0 is cut. range.snapshot.delegate.successes -> range.snapshots.delegate.successes range.snapshot.delegate.failures -> range.snapshots.delegate.failures
b2831cd
to
36ae825
Compare
bors r=kvoli |
Build failed: |
flake here #100119 bors r=kvoli |
Build succeeded: |
Adding the backport label after the PR has merged does nothing. You need to instruct blathers, as follows: blathers backport 23.1 |
Fixes: #98242
Knowing how many delegate snapshot requests are currently in-progress will be useful for detecting problems. This change adds a metric for this. It also updates the names of the previous stats to have the prefix
range.snapshots
vsrange.snapshot
to be consistent with other stats.Epic: none
Release note: None