-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
server,ui: Add debugging for quiesced ranges #26269
Conversation
LGTM, but you need to update |
3838684
to
d807a4e
Compare
pkg/storage/replica_test.go
Outdated
@@ -8672,7 +8672,7 @@ func TestReplicaMetrics(t *testing.T) { | |||
metrics := calcReplicaMetrics( | |||
context.Background(), hlc.Timestamp{}, config.SystemConfig{}, | |||
c.liveness, &c.desc, c.raftStatus, LeaseStatus{}, | |||
c.storeID, c.expected.Quiescent, CommandQueueMetrics{}, CommandQueueMetrics{}) | |||
c.storeID, c.expected.Quiescent, !c.expected.Quiescent, CommandQueueMetrics{}, CommandQueueMetrics{}) | |||
if c.expected != metrics { |
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.
I'm confused, shouldn't c.expected
need Ticking
set on it as well?
@@ -449,6 +453,7 @@ export default class RangeTable extends React.Component<RangeTableProps, {}> { | |||
problems: this.contentProblems(info.problems, awaitingGC), | |||
raftState: raftState, | |||
quiescent: info.quiescent ? rangeTableQuiescent : rangeTableEmptyContent, | |||
ticking: this.createContent(info.ticking.toString()), |
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.
I think it would be nice to roll this in with the quiescent row. I'm imagining we'd show quiescent and empty as normal states and "quiescent but ticking" and "not ticking but not quiescent" as error states.
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.
Yeah, I just wanted to get something in quickly for cyan without having to fiddle with and test all the combinations.
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.
I'm fine with that, we can just have @BramGruneir come through and add some niceties here.
I suspect that cockroachdb#26257 is caused by the unquiescedReplicas map introduced in cockroachdb#24956 getting out of sync with the per-replica quiescent flag. Add debug pages to help us see if that's happening. Release note: None
d807a4e
to
0a84d51
Compare
Just with @couchand's nits. Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending. Comments from Reviewable |
bors r+ |
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending. pkg/server/serverpb/status.proto, line 142 at r2 (raw file):
Even with the comment I was confused about whether the Comments from Reviewable |
26269: server,ui: Add debugging for quiesced ranges r=bdarnell a=bdarnell I suspect that #26257 is caused by the unquiescedReplicas map introduced in #24956 getting out of sync with the per-replica quiescent flag. Add debug pages to help us see if that's happening. Release note: None Co-authored-by: Ben Darnell <ben@cockroachlabs.com>
Build succeeded |
I suspect that #26257 is caused by the unquiescedReplicas map
introduced in #24956 getting out of sync with the per-replica
quiescent flag. Add debug pages to help us see if that's happening.
Release note: None