Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table ...................................................................... Patch Set 25: (13 comments) Nice work! I mostly have nit-picks. http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/client/schema.cc@211 PS25, Line 211: encoding Do we need to ToUpperCase() this here? Ah I see where this is used that we do that elsewhere. Maybe rename the "encoding" arg to "encoding_uc"? Or move the ToUpperCase() into this function? http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@5390 PS25, Line 5390: TEST_F(ToolTest, TestCreateTable) { nit: Maybe consider moving it into its own create_table-tool-test.cc that includes tool_test_util.h, especially if you implement Adar's fuzzing suggestion. I worry that the larger file size makes kudu-tool-test even harder to index, though I don't feel strongly about it. http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@5403 PS25, Line 5403: const string& master, : const string& table_name, : const string& schema, : const string& partition, : const map<string, string>& extra_configs, : KuduClient* client) { nit: add a few spaces to align with the ( http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@5412 PS25, Line 5412: Status s = RunKuduTool(table_args); : ASSERT_TRUE(s.ok()); : bool table_exists = false; : while (true) { : s = client->TableExists(table_name, &table_exists); : if (s.ok() || MonoTime::Now() > deadline) break; : SleepFor(MonoDelta::FromMilliseconds(10)); : } : ASSERT_TRUE(table_exists); nit: maybe be more succinct as: ASSERT_OK(RunKuduTool(table_args)); ASSERT_EVENTUALLY([&] { ASSERT_OK(client->TableExists(table_name, &table_exists)); ASSERT_TRUE(table_exists); }); This will retry the TableExists() call for up to 30 seconds with some backoff. That way we also don't need 'deadline'. http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@5422 PS25, Line 5422: CHECK_OK nit: ASSERT_OK so if it fails, it will fail more gracefully http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@5776 PS25, Line 5776: enum_type_with_str nit: What enum is this referring to? The bound_type? Maybe be more specific. Also . instead of , and same elsewhere http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@5838 PS25, Line 5838: invailed "invalid", same elsewhere http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@6104 PS25, Line 6104: : name:"; Does this print out "ID" too? http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@6424 PS25, Line 6424: error_encoding Isn't this also an error? http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/kudu-tool-test.cc@6551 PS25, Line 6551: a existed table nit: a table that already exists. http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/tool_action_table.cc@882 PS25, Line 882: KuduColumnStorageAttributes::AUTO_ENCODING; : break; : case ColumnPB::PLAIN_ENCODING : : *type = nit: there are some extra spaces here http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/tool_action_table.cc@931 PS25, Line 931: S nit: there's an extra space here http://gerrit.cloudera.org:8080/#/c/14306/25/src/kudu/tools/tool_action_table.cc@981 PS25, Line 981: Unknown integer value, nit: which integer are you referring to? Do you mean if the column has no encoding specified? -- To view, visit http://gerrit.cloudera.org:8080/14306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 25 Gerrit-Owner: YangSong <sy1...@yeah.net> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong <sy1...@yeah.net> Gerrit-Comment-Date: Wed, 23 Oct 2019 07:01:41 +0000 Gerrit-HasComments: Yes
