YangSong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14273 )

Change subject: add a tool to create table
......................................................................


Patch Set 10:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:

http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool.proto@342
PS7, Line 342:       optional string in_lower_bound = 1;
> New PBs shouldn't use 'required'.
use optional? But the table name is indispensable.


http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:

http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool_action_table.cc@881
PS7, Line 881:        type == "INT16" ||
             :        type == "INT32" ||
             :        type == "INT64" ||
             :        type == "UNIXTIME_MICROS") {
             :     int64_t v = 0;
             :
> As we discussed, let's always retrieve the JSON blob as a CLI argument. If
Today there's something wrong with the Internet. Now I update my code. Please 
help to review the code again. The tool can work now, but many test cases are 
missing. I think I need to generate many test create table JSON files in 
'src/kudu/tools/testdata/', and use them to test various scenarios. But I don't 
know how to start a kudu service in the test. Can you give me some help?



--
To view, visit http://gerrit.cloudera.org:8080/14273
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7
Gerrit-Change-Number: 14273
Gerrit-PatchSet: 10
Gerrit-Owner: YangSong <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: YangSong <[email protected]>
Gerrit-Comment-Date: Wed, 25 Sep 2019 10:16:54 +0000
Gerrit-HasComments: Yes

Reply via email to