You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/10/23 23:20:39 UTC
[kudu-CR] [tools] mention whitespace as a separator for variadic args
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16642
Change subject: [tools] mention whitespace as a separator for variadic args
......................................................................
[tools] mention whitespace as a separator for variadic args
I witnessed a case when people try to use a comma as separators for
a variadic argument in a kudu CLI command. As it turned out, the
help/usage message doesn't state clearly that multiple items in a
variadic argument should be separated by a whitespace. This changelist
updates the description of existing variadic arguments to explicitly
mention that the whitespace is the expected separator there.
Change-Id: I3ef979bd17655b5d46e1adb2a2f22923353ac435
---
M src/kudu/tools/tool_action_diagnose.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_remote_replica.cc
3 files changed, 52 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/16642/1
--
To view, visit http://gerrit.cloudera.org:8080/16642
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3ef979bd17655b5d46e1adb2a2f22923353ac435
Gerrit-Change-Number: 16642
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [tools] mention whitespace as a separator for variadic args
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16642 )
Change subject: [tools] mention whitespace as a separator for variadic args
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16642/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16642/2//COMMIT_MSG@10
PS2, Line 10: As it turned out, the
: help/usage message
In this particular case, was the user actually checking the help message to understand the issue?
Regardless, I suppose whatever error message they saw wasn't clear enough for them to try with whitespaces instead of commas. Maybe we should verify the args, especially if we know they are going to be UUIDs.
--
To view, visit http://gerrit.cloudera.org:8080/16642
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3ef979bd17655b5d46e1adb2a2f22923353ac435
Gerrit-Change-Number: 16642
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 23 Oct 2020 23:30:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] mention whitespace as a separator for variadic args
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16642
to look at the new patch set (#2).
Change subject: [tools] mention whitespace as a separator for variadic args
......................................................................
[tools] mention whitespace as a separator for variadic args
I witnessed a case when people try to use a comma as separators for
a variadic argument in a kudu CLI command. As it turned out, the
help/usage message doesn't state clearly that multiple items in a
variadic argument should be separated by a whitespace. This changelist
updates the description of existing variadic arguments to explicitly
mention that the whitespace is the expected separator there.
This patch does not contain any functional changes.
Change-Id: I3ef979bd17655b5d46e1adb2a2f22923353ac435
---
M src/kudu/tools/tool_action_diagnose.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_remote_replica.cc
3 files changed, 52 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/16642/2
--
To view, visit http://gerrit.cloudera.org:8080/16642
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3ef979bd17655b5d46e1adb2a2f22923353ac435
Gerrit-Change-Number: 16642
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [tools] mention whitespace as a separator for variadic args
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16642 )
Change subject: [tools] mention whitespace as a separator for variadic args
......................................................................
[tools] mention whitespace as a separator for variadic args
I witnessed a case when people try to use a comma as separators for
a variadic argument in a kudu CLI command. As it turned out, the
help/usage message doesn't state clearly that multiple items in a
variadic argument should be separated by a whitespace. This changelist
updates the description of existing variadic arguments to explicitly
mention that the whitespace is the expected separator there.
This patch does not contain any functional changes.
Change-Id: I3ef979bd17655b5d46e1adb2a2f22923353ac435
Reviewed-on: http://gerrit.cloudera.org:8080/16642
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/tools/tool_action_diagnose.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tools/tool_action_remote_replica.cc
3 files changed, 52 insertions(+), 46 deletions(-)
Approvals:
Andrew Wong: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/16642
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3ef979bd17655b5d46e1adb2a2f22923353ac435
Gerrit-Change-Number: 16642
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [tools] mention whitespace as a separator for variadic args
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16642 )
Change subject: [tools] mention whitespace as a separator for variadic args
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16642/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16642/2//COMMIT_MSG@10
PS2, Line 10: As it turned out, the
: help/usage message
> In this particular case, was the user actually checking the help message to
I don't know whether they looked into the help message or not after getting the 'Invalid argument' error. But that's what I did when they reported the issue :)
I guess they saw the following error message:
Invalid argument: Peer with uuid A,B is not in the committed config on this replica, rejecting the unsafe config change request for tablet T. Committed config: opid_index: -1 OBSOLETE_local: true peers { permanent_uuid: "UUID" member_type: VOTER last_known_addr { host: "a.b.c.d" port: PPPP } }
Yep, I guess the tool could first try to verify the args before issuing an RPC, but this is not what it does right now.
Anyways, I think it doesn't hurt to be more specific when describing the arguments, especially when we have other CLI commands with arguments expected to be comma-separated. Maybe, at some point we should switch to comma-separated format for all arguments having the list-like semantics?
Thank you for review!
--
To view, visit http://gerrit.cloudera.org:8080/16642
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3ef979bd17655b5d46e1adb2a2f22923353ac435
Gerrit-Change-Number: 16642
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 24 Oct 2020 00:35:32 +0000
Gerrit-HasComments: Yes