-
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
cli: limit flag for debug range-data / debug keys #49290
Conversation
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 4 of 4 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @tbg)
pkg/cli/debug.go, line 304 at r1 (raw file):
defer iter.Close() for i := 0; ; iter.Next() { i++
[nit] Move the i
declaration outside the for loop block, s/i/results, and move results++
closer to the limit check (similar to what we now have for runDebugKeys
).
1e4c5c8
to
0dc131e
Compare
@@ -105,7 +105,6 @@ func initCLIDefaults() { | |||
debugCtx.replicated = false | |||
debugCtx.inputFile = "" | |||
debugCtx.printSystemConfig = false | |||
debugCtx.maxResults = 1000 |
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.
Keep this line; it's needed. You can change the value to zero.
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.
done
pkg/cli/flags.go
Outdated
@@ -697,12 +697,14 @@ func init() { | |||
f := debugKeysCmd.Flags() | |||
VarFlag(f, (*mvccKey)(&debugCtx.startKey), cliflags.From) | |||
VarFlag(f, (*mvccKey)(&debugCtx.endKey), cliflags.To) | |||
IntFlag(f, &debugCtx.maxResults, cliflags.Limit, 0) |
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.
last arg is debugCtx.maxResults
, not 0
pkg/cli/flags.go
Outdated
BoolFlag(f, &debugCtx.values, cliflags.Values, debugCtx.values) | ||
BoolFlag(f, &debugCtx.sizes, cliflags.Sizes, debugCtx.sizes) | ||
} | ||
{ | ||
f := debugRangeDataCmd.Flags() | ||
BoolFlag(f, &debugCtx.replicated, cliflags.Replicated, debugCtx.replicated) | ||
IntFlag(f, &debugCtx.maxResults, cliflags.Limit, 0) |
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.
ditto
0dc131e
to
cf90575
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.
Reviewed 4 of 4 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei)
pkg/cli/flags.go, line 700 at r2 (raw file):
Previously, knz (kena) wrote…
last arg is
debugCtx.maxResults
, not0
that still needs to change (we don't want to reproduce defaults in multiple places)
pkg/cli/flags.go, line 707 at r2 (raw file):
Previously, knz (kena) wrote…
ditto
ditto
There was a teasing maxResults field already, completely unused. Release note: None
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif and @knz)
pkg/cli/debug.go, line 304 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
[nit] Move the
i
declaration outside the for loop block, s/i/results, and moveresults++
closer to the limit check (similar to what we now have forrunDebugKeys
).
done
pkg/cli/flags.go, line 700 at r2 (raw file):
Previously, knz (kena) wrote…
that still needs to change (we don't want to reproduce defaults in multiple places)
Done.
pkg/cli/flags.go, line 707 at r2 (raw file):
Previously, knz (kena) wrote…
ditto
Done.
cf90575
to
84681dd
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.
thx
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.
bors r+
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif and @knz)
Build succeeded |
There was a teasing maxResults field already, completely unused.
Release note: None