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

cli: limit flag for debug range-data / debug keys #49290

Merged
merged 1 commit into from
Jun 1, 2020

Conversation

andreimatei
Copy link
Contributor

There was a teasing maxResults field already, completely unused.

Release note: None

@andreimatei andreimatei requested a review from a team as a code owner May 19, 2020 21:30
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@irfansharif irfansharif 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 4 of 4 files at r1.
Reviewable status: :shipit: 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).

@@ -105,7 +105,6 @@ func initCLIDefaults() {
debugCtx.replicated = false
debugCtx.inputFile = ""
debugCtx.printSystemConfig = false
debugCtx.maxResults = 1000
Copy link
Contributor

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.

Copy link
Contributor Author

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)
Copy link
Contributor

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@tbg tbg removed their request for review May 26, 2020 09:30
Copy link
Contributor

@knz knz left a 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: :shipit: 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, not 0

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
Copy link
Contributor Author

@andreimatei andreimatei 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 @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 move results++ closer to the limit check (similar to what we now have for runDebugKeys).

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.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

thx

Copy link
Contributor Author

@andreimatei andreimatei left a 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: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif and @knz)

@craig
Copy link
Contributor

craig bot commented Jun 1, 2020

Build succeeded

@craig craig bot merged commit 6b7b413 into cockroachdb:master Jun 1, 2020
@andreimatei andreimatei deleted the misc.dump-limit branch June 1, 2020 19:29
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