ui: display very large ranges in problematic ranges #129001
Conversation
|
This change is |
iskettaneh
force-pushed
the
TooLargeRange
branch
2 times, most recently
from
4e5e6c8 to
cb0219e
Compare
iskettaneh
requested review from
angles-n-daemons
and removed request for
a team
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @angles-n-daemons, @arulajmani, and @iskettaneh)
pkg/kv/kvserver/replica_metrics.go line 368 at r1 (raw file):
} func (r *Replica) splitSizeRLocked() (maxBytes int64) {
Can you add a comment for this new function?
pkg/kv/kvserver/replica_metrics_test.go line 60 at r1 (raw file):
ctr, down, under, over, _ := calcRangeCounter(1100, threeVotersAndSingleNonVoter, leaseStatus, livenesspb.NodeVitalityMap{ 1000: livenesspb.FakeNodeVitality(true), // by NodeID }, 3 /* numVoters */, 4 /* numReplicas */, 4 /* clusterNodes */, 0 /*rangeTooLargeThreshold*/, 0 /*rangeSize*/)
nit: I think usually we put spaces within the comment, like /* rangeTooLargeThreshold */.
There was a problem hiding this comment.
Reviewed 2 of 9 files at r1, all commit messages.
Reviewable status:
pkg/kv/kvserver/replica_metrics.go line 174 at r1 (raw file):
const ( raftLogTooLargeMultiple = 4 rangeTooLargeMultiple = 8
Should this be 2, given we set backpressureRangeSizeMultiplier to be 2 by default?
pkg/kv/kvserver/replica_metrics.go line 176 at r1 (raw file):
rangeTooLargeMultiple = 8 ) largeRangeThreshold := rangeTooLargeMultiple * d.rangeSplitSize
See my comment below, but I think we should just simplify this to be d.conf.MaxRangeBytes.
pkg/kv/kvserver/replica_metrics.go line 370 at r1 (raw file):
func (r *Replica) splitSizeRLocked() (maxBytes int64) { maxBytes = r.mu.conf.RangeMaxBytes if r.mu.largestPreviousMaxRangeSizeBytes > maxBytes {
This calculation is pretty contained to exceedsSplitSizeRLocked. IMO, we shouldn't let this bleed in elsewhere -- how do you feel about reverting this change and just using RangeMaxBytes in the calculation for whether a range is too large or not?
pkg/kv/kvserver/replica_metrics_test.go line 60 at r1 (raw file):
Previously, miraradeva (Mira Radeva) wrote…
nit: I think usually we put spaces within the comment, like
/* rangeTooLargeThreshold */.
+1. Elsewhere in this file as well.
iskettaneh
force-pushed
the
TooLargeRange
branch
from
cb0219e to
f526876
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/replica_metrics.go line 174 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Should this be 2, given we set
backpressureRangeSizeMultiplierto be 2 by default?
Done.
pkg/kv/kvserver/replica_metrics.go line 176 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
See my comment below, but I think we should just simplify this to be d.conf.MaxRangeBytes.
Done.
pkg/kv/kvserver/replica_metrics.go line 368 at r1 (raw file):
Previously, miraradeva (Mira Radeva) wrote…
Can you add a comment for this new function?
Removed the functions based on Arul's comments.
pkg/kv/kvserver/replica_metrics.go line 370 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
This calculation is pretty contained to
exceedsSplitSizeRLocked. IMO, we shouldn't let this bleed in elsewhere -- how do you feel about reverting this change and just usingRangeMaxBytesin the calculation for whether a range is too large or not?
Sounds good to me.
There was a problem hiding this comment.
Reviewable status:
-- commits line 12 at r2:
This probably deserves a release note like Release note(ui): .... See https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/186548364/Release+notes.
iskettaneh
force-pushed
the
TooLargeRange
branch
from
f526876 to
aa28fde
Compare
There was a problem hiding this comment.
Reviewable status:
Previously, miraradeva (Mira Radeva) wrote…
This probably deserves a release note like
Release note(ui): .... See https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/186548364/Release+notes.
Done.
There was a problem hiding this comment.
Reviewed 5 of 9 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status:
pkg/kv/kvserver/replica_metrics.go line 114 at r3 (raw file):
raftLogSizeTrusted: r.mu.raftLogSizeTrusted, rangeSize: r.mu.state.Stats.Total(), rangeSplitSize: r.mu.conf.RangeMaxBytes,
Is there benefit to carrying copying over this into rangeSplitSize and then using it below? Or, could we get rid of this field entirely and use d.conf.MaxRangeBytes in calcReplicaMetrics instead?
iskettaneh
force-pushed
the
TooLargeRange
branch
from
aa28fde to
64fcd4d
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/replica_metrics.go line 114 at r3 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Is there benefit to carrying copying over this into
rangeSplitSizeand then using it below? Or, could we get rid of this field entirely and used.conf.MaxRangeBytesincalcReplicaMetricsinstead?
My bad, I didn't notice that r.mu.conf is passed already.
Done!
This commit adds a column in the DB Console's page problematic ranges that displays the ranges that are too large. The threshold is set to be: 8 * the current range max size. Fixes: cockroachdb#127843 Epic: None Release note (ui change): Adding too large ranges to the problematic ranges page in DB Console. If a range is larger than twice the max range size, it will show up in the problematic ranges page.
iskettaneh
force-pushed
the
TooLargeRange
branch
from
64fcd4d to
b0e1aaa
Compare
|
bors r+ |
This commit adds a column in the DB Console's page problematic ranges that displays the ranges that are too large. The threshold is set to be: 8 * the current range max size.
The two screenshots below show how that looks like. In order to generate that, I had to set the threshold multiplier to be 0.8 (instead of 8).
Fixes: #127843
Epic: None
Release note (ui change): Adding too large ranges to the problematic ranges
page in DB Console. If a range is larger than twice the max range
size, it will show up in the problematic ranges page.