You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2018/10/12 10:26:18 UTC
[kudu-CR] [tools] Add locate row tool
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11666
Change subject: [tools] Add locate row tool
......................................................................
[tools] Add locate row tool
Sometimes while debugging I find it frustrating that it's very difficult
to tell what tablet a particular row belongs to. This basic tool
provides a way to find out, by providing a simple interface that accepts
a primary key as a CSV and will print out the table id of the
corresponding tablet, or an error if there is no such tablet.
A note about tests: it's difficult to verify the answer of the tool
independently. However, the implementation is just scan tokens, so the
tablet-finding logic should be well-exercised by lots of other client
tests. Consequently, the tests focus on error cases and sanity checks.
Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 348 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/11666/1
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 6:
Failure looks unrelated.
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 12 Oct 2018 20:04:47 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] Add locate row tool
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11666/9/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/11666/9/src/kudu/tools/tool_action_table.cc@216
PS9, Line 216: switch (type) {
> none of those can be in a PK because they are not lexicographically sortabl
Ah I see float, double, and bool aren't supported. This first paragraph here makes it seem like decimal is? https://kudu.apache.org/docs/schema_design.html#decimal
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 13 Oct 2018 04:49:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 9: Code-Review+2
LGTM as long as Andrew is good w/ it
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 13 Oct 2018 01:26:42 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] Add locate row tool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 12: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 12
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 16 Oct 2018 17:55:40 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 10:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11666/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11666/10//COMMIT_MSG@10
PS10, Line 10: a particular row belongs to
> Does it make sense to introduce an option to check whether that particular
Yeah, good idea. Let me post that as a quick follow up.
http://gerrit.cloudera.org:8080/#/c/11666/10//COMMIT_MSG@29
PS10, Line 29: ,
> nit: I think it's supposed to be a column (':') instead of comma (',').
Done
http://gerrit.cloudera.org:8080/#/c/11666/10/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/11666/10/src/kudu/tools/kudu-admin-test.cc@1513
PS10, Line 1513: String instead of int
> Just curious: what is the output for '0x10'?
That's a parse error as JSON does not recognize hex literals as valid number literals. See json.org for the a DFA-ish depiction of the number parsing. Also, I checked it is in fact a parse error.
http://gerrit.cloudera.org:8080/#/c/11666/10/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/11666/10/src/kudu/tools/tool_action_table.cc@226
PS10, Line 226: ExtractInt64
> I'm not sure this works for small negative numbers due to the funkiness of
Wow good catch Alexey. I think that must be a bug in jsonreader? I will rebase this patch on top of a patch to fix that
http://gerrit.cloudera.org:8080/#/c/11666/10/src/kudu/tools/tool_action_table.cc@351
PS10, Line 351: be
> nit: 'be' --> 'will be'?
Done
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 10
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 07:35:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11666/9/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/11666/9/src/kudu/tools/tool_action_table.cc@216
PS9, Line 216: switch (type) {
What about the other column types? DECIMAL, FLOAT, DOUBLE, BOOL?
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 13 Oct 2018 00:09:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 10: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11666/9/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/11666/9/src/kudu/tools/tool_action_table.cc@216
PS9, Line 216: const auto& col
> Ah, you're right. Decimal is a problem because you have to use int128 to us
SGTM
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 10
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 13 Oct 2018 19:28:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11666
to look at the new patch set (#9).
Change subject: [tools] Add locate row tool
......................................................................
[tools] Add locate row tool
Sometimes while debugging I find it frustrating that it's very difficult
to tell what tablet a particular row belongs to. This basic tool
provides a way to find out, by providing a simple interface that accepts
a primary key as a JSON array and will print out the tablet id of the
corresponding tablet, or an error if there is no such tablet.
For example, with a table created like
CREATE TABLE test (
key0 STRING NOT NULL,
key1 INT32 NOT NULL,
PRIMARY KEY(key0, key1)
)
an invocation of the tool looks like
$ kudu table locate_row localhost:7053 test "[\"foo\", 2]"
The choice of a JSON array is a compromise between CSV, which is easiest
and fastest to type in the common case, and a verbose JSON object like
{ "key0" : "foo", "key1", 2 }, which is most explicit but long and
difficult to type on the command line. A JSON array has the benefit of
having well-defined escaping rules and formatting while being almost as
easy to type out as CSV.
A note about tests: it's difficult to verify the answer of the tool
independently. However, the implementation is just scan tokens, so the
tablet-finding logic should be well-exercised by lots of other client
tests. Consequently, the tests focus on error cases and sanity checks.
Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 383 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/11666/9
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] Add locate row tool
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11666/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11666/6//COMMIT_MSG@12
PS6, Line 12: CSV
didn't look at the patch, but from a glance at the commit message: did you consider JSON? The downside of CSV is that it's completely non-standardized how to handle escaping of quotes, commas, newlines, etc.
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 12 Oct 2018 18:43:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 13:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/kudu-admin-test.cc@1502
PS13, Line 1502: // Grab list of tablet_ids from any tserver and check the output.
Yeah but there's only one tserver.
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/kudu-admin-test.cc@1512
PS13, Line 1512: // Test a couple of error cases.
Maybe consolidate the bulk of this into a lambda so it's not so verbose?
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/kudu-admin-test.cc@1598
PS13, Line 1598: // Since there isn't a great alternative way to validate the answer the tool
A lambda might help here too.
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc@200
PS13, Line 200: nullptr
Nit: /*field=*/ nullptr
Below too.
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc@250
PS13, Line 250: case KuduColumnSchema::DECIMAL:
If you're going to special-case DECIMAL as a "known" unsupported type, shouldn't you also special-case BOOL, FLOAT, and DOUBLE?
Also, is this short-circuit actually necessary? Isn't it impossible to see those types here? And if we do, isn't it a sign that this is a future version of Kudu that supports these types in the primary key and could accept this predicate?
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc@282
PS13, Line 282: "all primary key columns specified but more than one matching tablet?");
Not that this is possible, but perhaps dump all of the matching tablet IDs?
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc@355
PS13, Line 355: .AddRequiredParameter({ kKeyArg,
What do you think about using AddRequiredVariadicParameter here, so that when using on the command line you don't need to quote the entire array in order for the tool to marshal it as a single argument?
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 13
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 17 Oct 2018 04:03:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11666/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11666/10//COMMIT_MSG@10
PS10, Line 10: a particular row belongs to
Does it make sense to introduce an option to check whether that particular row actually exists in the table?
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 10
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 04:47:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 12 Oct 2018 20:04:54 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11666
to look at the new patch set (#4).
Change subject: [tools] Add locate row tool
......................................................................
[tools] Add locate row tool
Sometimes while debugging I find it frustrating that it's very difficult
to tell what tablet a particular row belongs to. This basic tool
provides a way to find out, by providing a simple interface that accepts
a primary key as a CSV and will print out the tablet id of the
corresponding tablet, or an error if there is no such tablet.
A note about tests: it's difficult to verify the answer of the tool
independently. However, the implementation is just scan tokens, so the
tablet-finding logic should be well-exercised by lots of other client
tests. Consequently, the tests focus on error cases and sanity checks.
Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 349 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/11666/4
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 13:
(7 comments)
New changelist for the stuff I did: http://gerrit.cloudera.org:8080/11715
I'll post a second one for the BOOL, DOUBLE, FLOAT future-proofing.
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/kudu-admin-test.cc@1502
PS13, Line 1502: // Grab list of tablet_ids from any tserver and check the output.
> Yeah but there's only one tserver.
Done
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/kudu-admin-test.cc@1512
PS13, Line 1512: // Test a couple of error cases.
> Maybe consolidate the bulk of this into a lambda so it's not so verbose?
Done
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/kudu-admin-test.cc@1598
PS13, Line 1598: // Since there isn't a great alternative way to validate the answer the tool
> A lambda might help here too.
These two do some different checks at the end and there's only two.
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc@200
PS13, Line 200: nullptr
> Nit: /*field=*/ nullptr
Done
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc@250
PS13, Line 250: case KuduColumnSchema::DECIMAL:
> If you're going to special-case DECIMAL as a "known" unsupported type, shou
DECIMAL is actually a supported PK type, but it doesn't have a representation in JSON so we'd have to invent one, and it's not straightforward to just use an integer to represent a decimal value because the int would need to be 128-bit and our json reading library doesn't support that.
For the other types, yeah, we could future-proof the tool by supporting them here.
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc@282
PS13, Line 282: "all primary key columns specified but more than one matching tablet?");
> Not that this is possible, but perhaps dump all of the matching tablet IDs?
Done
http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc@355
PS13, Line 355: .AddRequiredParameter({ kKeyArg,
> What do you think about using AddRequiredVariadicParameter here, so that wh
Doesn't seem to work. For example, consider
kudu table locate_row my_master my_table [ "foo bar" ]
The tool will make the variadic args be {[, foo bar, ]}, so joining them together again gives [foo bar], which is bad json. If you escape the quotation marks, you get {[, "foo, bar",]}, which when concatenated again is [foobar], which is wrong.
So on the one hand we need to escape quotation marks to handle strings properly, but then it does the wrong thing by breaking at spaces in the literal.
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 13
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 17 Oct 2018 22:17:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11666
to look at the new patch set (#8).
Change subject: [tools] Add locate row tool
......................................................................
[tools] Add locate row tool
Sometimes while debugging I find it frustrating that it's very difficult
to tell what tablet a particular row belongs to. This basic tool
provides a way to find out, by providing a simple interface that accepts
a primary key as a JSON array and will print out the tablet id of the
corresponding tablet, or an error if there is no such tablet.
For example, with a table created like
CREATE TABLE test (
key0 STRING NOT NULL,
key1 INT32 NOT NULL,
PRIMARY KEY(key0, key1)
)
an invocation of the tool looks like
$ kudu table locate_row localhost:7053 test "[\"foo\", 2]"
The choice of a JSON array is a compromise between CSV, which is easiest
and fastest to type in the common case, and a verbose JSON object like
{ "key0" : "foo", "key1", 2 }, which is most explicit but long and
difficult to type on the command line. A JSON array has the benefit of
having well-defined escaping rules and formatting while being almost as
easy to type out as CSV.
A note about tests: it's difficult to verify the answer of the tool
independently. However, the implementation is just scan tokens, so the
tablet-finding logic should be well-exercised by lots of other client
tests. Consequently, the tests focus on error cases and sanity checks.
Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 383 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/11666/8
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] Add locate row tool
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11666/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11666/6//COMMIT_MSG@12
PS6, Line 12: CSV
> The downside of JSON is that it is verbose and cumbersome to type in the co
I buy that. Would JSON be less cumbersome if we just took it as a JSON array instead of dictionary? eg kudu locate '["foo", "bar", 123]' ?
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 12 Oct 2018 19:35:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11666
to look at the new patch set (#11).
Change subject: [tools] Add locate row tool
......................................................................
[tools] Add locate row tool
Sometimes while debugging I find it frustrating that it's very difficult
to tell what tablet a particular row belongs to. This basic tool
provides a way to find out, by providing a simple interface that accepts
a primary key as a JSON array and will print out the tablet id of the
corresponding tablet, or an error if there is no such tablet.
For example, with a table created like
CREATE TABLE test (
key0 STRING NOT NULL,
key1 INT32 NOT NULL,
PRIMARY KEY(key0, key1)
)
an invocation of the tool looks like
$ kudu table locate_row localhost:7053 test "[\"foo\", 2]"
The choice of a JSON array is a compromise between CSV, which is easiest
and fastest to type in the common case, and a verbose JSON object like
{ "key0" : "foo", "key1" : 2 }, which is most explicit but long and
difficult to type on the command line. A JSON array has the benefit of
having well-defined escaping rules and formatting while being almost as
easy to type out as CSV.
A note about tests: it's difficult to verify the answer of the tool
independently. However, the implementation is just scan tokens, so the
tablet-finding logic should be well-exercised by lots of other client
tests. Consequently, the tests focus on error cases and sanity checks.
Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 391 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/11666/11
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 11
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11666/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11666/6//COMMIT_MSG@12
PS6, Line 12: CSV
> I buy that. Would JSON be less cumbersome if we just took it as a JSON arra
Yeah, that's a happy medium that solves the standardization and escaping problem while being pretty easy to type.
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 12 Oct 2018 19:51:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 10:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11666/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11666/10//COMMIT_MSG@29
PS10, Line 29: ,
nit: I think it's supposed to be a column (':') instead of comma (',').
http://gerrit.cloudera.org:8080/#/c/11666/10/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/11666/10/src/kudu/tools/kudu-admin-test.cc@1513
PS10, Line 1513: String instead of int
Just curious: what is the output for '0x10'?
http://gerrit.cloudera.org:8080/#/c/11666/10/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/11666/10/src/kudu/tools/tool_action_table.cc@226
PS10, Line 226: ExtractInt64
I'm not sure this works for small negative numbers due to the funkiness of jsonreader wrapper (i.e. if the number fits into int16_t). Did you try negative keys like '-1' with your new tool? I'm curious how it works in that case.
http://gerrit.cloudera.org:8080/#/c/11666/10/src/kudu/tools/tool_action_table.cc@351
PS10, Line 351: be
nit: 'be' --> 'will be'?
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 10
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 04:39:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11666
to look at the new patch set (#2).
Change subject: [tools] Add locate row tool
......................................................................
[tools] Add locate row tool
Sometimes while debugging I find it frustrating that it's very difficult
to tell what tablet a particular row belongs to. This basic tool
provides a way to find out, by providing a simple interface that accepts
a primary key as a CSV and will print out the tablet id of the
corresponding tablet, or an error if there is no such tablet.
A note about tests: it's difficult to verify the answer of the tool
independently. However, the implementation is just scan tokens, so the
tablet-finding logic should be well-exercised by lots of other client
tests. Consequently, the tests focus on error cases and sanity checks.
Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 348 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/11666/2
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11666
to look at the new patch set (#10).
Change subject: [tools] Add locate row tool
......................................................................
[tools] Add locate row tool
Sometimes while debugging I find it frustrating that it's very difficult
to tell what tablet a particular row belongs to. This basic tool
provides a way to find out, by providing a simple interface that accepts
a primary key as a JSON array and will print out the tablet id of the
corresponding tablet, or an error if there is no such tablet.
For example, with a table created like
CREATE TABLE test (
key0 STRING NOT NULL,
key1 INT32 NOT NULL,
PRIMARY KEY(key0, key1)
)
an invocation of the tool looks like
$ kudu table locate_row localhost:7053 test "[\"foo\", 2]"
The choice of a JSON array is a compromise between CSV, which is easiest
and fastest to type in the common case, and a verbose JSON object like
{ "key0" : "foo", "key1", 2 }, which is most explicit but long and
difficult to type on the command line. A JSON array has the benefit of
having well-defined escaping rules and formatting while being almost as
easy to type out as CSV.
A note about tests: it's difficult to verify the answer of the tool
independently. However, the implementation is just scan tokens, so the
tablet-finding logic should be well-exercised by lots of other client
tests. Consequently, the tests focus on error cases and sanity checks.
Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 391 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/11666/10
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 10
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
[tools] Add locate row tool
Sometimes while debugging I find it frustrating that it's very difficult
to tell what tablet a particular row belongs to. This basic tool
provides a way to find out, by providing a simple interface that accepts
a primary key as a JSON array and will print out the tablet id of the
corresponding tablet, or an error if there is no such tablet.
For example, with a table created like
CREATE TABLE test (
key0 STRING NOT NULL,
key1 INT32 NOT NULL,
PRIMARY KEY(key0, key1)
)
an invocation of the tool looks like
$ kudu table locate_row localhost:7053 test "[\"foo\", 2]"
The choice of a JSON array is a compromise between CSV, which is easiest
and fastest to type in the common case, and a verbose JSON object like
{ "key0" : "foo", "key1" : 2 }, which is most explicit but long and
difficult to type on the command line. A JSON array has the benefit of
having well-defined escaping rules and formatting while being almost as
easy to type out as CSV.
A note about tests: it's difficult to verify the answer of the tool
independently. However, the implementation is just scan tokens, so the
tablet-finding logic should be well-exercised by lots of other client
tests. Consequently, the tests focus on error cases and sanity checks.
Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Reviewed-on: http://gerrit.cloudera.org:8080/11666
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 391 insertions(+), 1 deletion(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 13
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Alexey Serbin, Attila Bukor, Andrew Wong, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11666
to look at the new patch set (#7).
Change subject: [tools] Add locate row tool
......................................................................
[tools] Add locate row tool
Sometimes while debugging I find it frustrating that it's very difficult
to tell what tablet a particular row belongs to. This basic tool
provides a way to find out, by providing a simple interface that accepts
a primary key as a JSON array and will print out the tablet id of the
corresponding tablet, or an error if there is no such tablet.
For example, with a table created like
CREATE TABLE test (
key0 STRING NOT NULL,
key1 INT32 NOT NULL,
PRIMARY KEY(key0, key1)
)
an invocation of the tool looks like
$ kudu table locate_row localhost:7053 test "[\"foo\", 2]"
The choice of a JSON array is a compromise between CSV, which is easiest
and fastest to type in the common case, and a verbose JSON object like
{ "key0" : "foo", "key1", 2 }, which is most explicit but long and
difficult to type on the command line. A JSON array has the benefit of
having well-defined escaping rules and formatting while being almost as
easy to type out as CSV.
A note about tests: it's difficult to verify the answer of the tool
independently. However, the implementation is just scan tokens, so the
tablet-finding logic should be well-exercised by lots of other client
tests. Consequently, the tests focus on error cases and sanity checks.
Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
---
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_table.cc
2 files changed, 381 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/11666/7
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [tools] Add locate row tool
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 7: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11666/7/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/11666/7/src/kudu/tools/tool_action_table.cc@337
PS7, Line 337: Provide the primary key as a comma-separated list of "
: "primary key values, e.g. 1,foo,2,bar.
update this. Also I think worth providing an example like '[3, "foo"]'
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 12 Oct 2018 21:39:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11666/9/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/11666/9/src/kudu/tools/tool_action_table.cc@216
PS9, Line 216: switch (type) {
> What about the other column types? DECIMAL, FLOAT, DOUBLE, BOOL?
none of those can be in a PK because they are not lexicographically sortable
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 13 Oct 2018 01:25:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 11: Code-Review+2
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11666/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11666/10//COMMIT_MSG@10
PS10, Line 10: a particular row belongs to
> Yeah, good idea. Let me post that as a quick follow up.
Thank you for addressing that promptly!
http://gerrit.cloudera.org:8080/#/c/11666/10/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/11666/10/src/kudu/tools/kudu-admin-test.cc@1513
PS10, Line 1513: String instead of int
> That's a parse error as JSON does not recognize hex literals as valid numbe
Thanks for checking!
http://gerrit.cloudera.org:8080/#/c/11666/10/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/11666/10/src/kudu/tools/tool_action_table.cc@226
PS10, Line 226: ExtractInt64
> Wow good catch Alexey. I think that must be a bug in jsonreader? I will reb
Thanks a lot!
Yep, I remember I looked at the jsonreader and was a bit curious about GetUIntXXX() calls and negative numbers, but was too lazy to check that out that time.
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 11
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 18:04:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11666/9/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/11666/9/src/kudu/tools/tool_action_table.cc@216
PS9, Line 216: switch (type) {
> Ah I see float, double, and bool aren't supported. This first paragraph her
Ah, you're right. Decimal is a problem because you have to use int128 to use it and there's no parsing for ints that big. I'm gonna punt on it for now and hopefully get around to it in a follow-up.
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 13 Oct 2018 15:59:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11666/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11666/6//COMMIT_MSG@12
PS6, Line 12: CSV
> didn't look at the patch, but from a glance at the commit message: did you
The downside of JSON is that it is verbose and cumbersome to type in the command line without making a mistake. Maybe the right path is to have the default be CSV, because it's easy to use, but also accept JSON via a flag to handle cases when the primary key has values that are difficult to enter with CSV.
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 12 Oct 2018 18:49:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Add locate row tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11666/7/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/11666/7/src/kudu/tools/tool_action_table.cc@337
PS7, Line 337: Provide the primary key as a comma-separated list of "
: "primary key values, e.g. 1,foo,2,bar.
> update this. Also I think worth providing an example like '[3, "foo"]'
Done
--
To view, visit http://gerrit.cloudera.org:8080/11666
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3
Gerrit-Change-Number: 11666
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 12 Oct 2018 22:40:29 +0000
Gerrit-HasComments: Yes