You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Sam Okrent (Code Review)" <ge...@cloudera.org> on 2017/05/25 02:03:55 UTC
[kudu-CR] KUDU-1911 add more helpful error message when required tool arguments are missing
Sam Okrent has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6986
Change subject: KUDU-1911 add more helpful error message when required tool arguments are missing
......................................................................
KUDU-1911 add more helpful error message when required tool arguments are missing
When using a tool that has a required positional argument, excluding that
argument or passing it as a flag caused a confusing error message to be
printed out ("must provide missing_argument"). This commit changes
the message to "must provide positional argument missing_argument".
Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_main.cc
2 files changed, 18 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/6986/1
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
[kudu-CR] KUDU-1911 improve missing required arg message
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: KUDU-1911 improve missing required arg message
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6986/4/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
PS4, Line 513: "master_addresses"));
: NO_FATALS(RunActionMissingRequiredArg("local_replica cmeta rewrite_raft_config fake_id",
:
nit: spacing
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1911 improve missing required arg message
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: KUDU-1911 improve missing required arg message
......................................................................
Patch Set 6: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1911 improve missing required arg message
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1911 improve missing required arg message
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1911 improve missing required arg message
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: KUDU-1911 improve missing required arg message
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6986/4/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
PS4, Line 228: ide positional argument $0";
: const string kVariadicArgumentMessage = "must provide
nit: it may be better for readability to move these "$0" into the call to Substitute()
I.e. Substitute("$0 $1", message, required_arg)
PS4, Line 515:
nit: can you put a /* variadic */ here?
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1911 improve missing required arg message
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: KUDU-1911 improve missing required arg message
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6986/3//COMMIT_MSG
Commit Message:
PS3, Line 11: his commit changes
: the message to "must provide positional argument mi
should be updated with the "variadic" change
http://gerrit.cloudera.org:8080/#/c/6986/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
PS3, Line 226: redArgu
nit: RunActionMissingRequiredArg()
PS3, Line 234: :InvalidArgument(
: Substitute(message, required_arg));
does this still need to line wrap?
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1911 improve missing required arg message
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: KUDU-1911 improve missing required arg message
......................................................................
KUDU-1911 improve missing required arg message
When using a tool that has a required positional argument, excluding that
argument or passing it as a flag caused a confusing error message to be
printed out ("must provide missing_argument"). This commit changes
the message to "must provide positional argument missing_argument" for
non-variadic arguments and "must provide variadic positional argument
missing_argument" for variadic arguments.
Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Reviewed-on: http://gerrit.cloudera.org:8080/6986
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Dan Burkert <da...@apache.org>
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_main.cc
2 files changed, 24 insertions(+), 2 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Andrew Wong: Looks good to me, but someone else must approve
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1911 improve missing required arg message
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6986
to look at the new patch set (#5).
Change subject: KUDU-1911 improve missing required arg message
......................................................................
KUDU-1911 improve missing required arg message
When using a tool that has a required positional argument, excluding that
argument or passing it as a flag caused a confusing error message to be
printed out ("must provide missing_argument"). This commit changes
the message to "must provide positional argument missing_argument" for
non-variadic arguments and "must provide variadic positional argument
missing_argument" for variadic arguments.
Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_main.cc
2 files changed, 24 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/6986/5
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1911 add more helpful error message when required tool arguments are missing
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: KUDU-1911 add more helpful error message when required tool arguments are missing
......................................................................
Patch Set 1:
(3 comments)
left a couple formatting comments
http://gerrit.cloudera.org:8080/#/c/6986/1//COMMIT_MSG
Commit Message:
Line 7: KUDU-1911 add more helpful error message when required tool arguments are missing
nit: from https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines:
"As a general rule, your messages should start with a single line that’s no more than about 50 characters and that describes the changeset concisely, followed by a blank line, followed by a more detailed explanation."
http://gerrit.cloudera.org:8080/#/c/6986/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
Line 508: TEST_F(ToolTest, TestActionRequiredArgument) {
maybe TestActionMissingRequiredArg?
Line 510: NO_FATALS(RunActionRequiredArgument("cluster ksck --master_addresses=master.example.com", "master_addresses"));
nit: keep the line length under 100 characters
See this part of the style guide for spacing tips:
https://google.github.io/styleguide/cppguide.html#Formatting
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1911 improve missing required arg message
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6986
to look at the new patch set (#6).
Change subject: KUDU-1911 improve missing required arg message
......................................................................
KUDU-1911 improve missing required arg message
When using a tool that has a required positional argument, excluding that
argument or passing it as a flag caused a confusing error message to be
printed out ("must provide missing_argument"). This commit changes
the message to "must provide positional argument missing_argument" for
non-variadic arguments and "must provide variadic positional argument
missing_argument" for variadic arguments.
Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_main.cc
2 files changed, 24 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/6986/6
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1911 improve missing required arg message
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1911 improve missing required arg message
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1911 improve missing required arg message
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6986
to look at the new patch set (#4).
Change subject: KUDU-1911 improve missing required arg message
......................................................................
KUDU-1911 improve missing required arg message
When using a tool that has a required positional argument, excluding that
argument or passing it as a flag caused a confusing error message to be
printed out ("must provide missing_argument"). This commit changes
the message to "must provide positional argument missing_argument" for
non-variadic arguments and "must provide variadic positional argument
missing_argument" for variadic arguments.
Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_main.cc
2 files changed, 24 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/6986/4
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1911 add more helpful error message when required tool arguments are missing
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1911 add more helpful error message when required tool arguments are missing
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6986/1/src/kudu/tools/tool_main.cc
File src/kudu/tools/tool_main.cc:
Line 95: return Status::InvalidArgument(Substitute("must provide $0", a.name));
Consider adding 'flag argument' to this, in order to further differentiate.
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1911 improve missing required arg message
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: KUDU-1911 improve missing required arg message
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6986/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
PS5, Line 227: bo
nit: spacing, didn't catch it last time
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1911 improve missing required arg message
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6986
to look at the new patch set (#3).
Change subject: KUDU-1911 improve missing required arg message
......................................................................
KUDU-1911 improve missing required arg message
When using a tool that has a required positional argument, excluding that
argument or passing it as a flag caused a confusing error message to be
printed out ("must provide missing_argument"). This commit changes
the message to "must provide positional argument missing_argument".
Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_main.cc
2 files changed, 25 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/6986/3
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1911 improve missing required arg message
Posted by "Sam Okrent (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6986
to look at the new patch set (#2).
Change subject: KUDU-1911 improve missing required arg message
......................................................................
KUDU-1911 improve missing required arg message
When using a tool that has a required positional argument, excluding that
argument or passing it as a flag caused a confusing error message to be
printed out ("must provide missing_argument"). This commit changes
the message to "must provide positional argument missing_argument".
Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_main.cc
2 files changed, 25 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/6986/2
--
To view, visit http://gerrit.cloudera.org:8080/6986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2128a01da5c4929981b2ea46a32b0f7b9dd066e1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sam Okrent <sa...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins