Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13833 )
Change subject: KUDU-2881 Support create/drop range partition by command line ...................................................................... Patch Set 15: (6 comments) http://gerrit.cloudera.org:8080/#/c/13833/15//COMMIT_MSG Commit Message: PS15: Could you restore the commit message indentation as it was before? git will automatically indent the message after the commit is merged. http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2350 PS15, Line 2350: Status InsertTestRows(const client::sp::shared_ptr<KuduClient>& client, This function should be declared in an anonymous namespace, or declared static. It's also indented too much. http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2372 PS15, Line 2372: RETURN_NOT_OK(session->Flush()); : RETURN_NOT_OK(session->Close()); This isn't necessary; what is being flushed here? http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2426 PS15, Line 2426: ASSERT_TRUE(s.ok()) << ToolRunInfo(s, stdout, stderr); ASSERT_OK(s) << ... would be more idiomatic. http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2529 PS15, Line 2529: Status s = InsertTestRows(client_, kTestTableName, value, 1); : EXPECT_TRUE(s.IsNotFound()); You can combine this into one line. http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/tool_action_table.cc@575 PS15, Line 575: case KuduColumnSchema::BOOL: FALLTHROUGH_INTENDED; : case KuduColumnSchema::FLOAT: FALLTHROUGH_INTENDED; : case KuduColumnSchema::DOUBLE: FALLTHROUGH_INTENDED; : case KuduColumnSchema::DECIMAL: FALLTHROUGH_INTENDED; As I mentioned in my earlier comment here, for "direct chaining" (i.e. cases that don't execute a single line of code), you don't need to add FALLTHROUGH_INTENDED. It's only necessary when the case does something and then falls through. -- To view, visit http://gerrit.cloudera.org:8080/13833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 15 Gerrit-Owner: honeyhexin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu <[email protected]> Gerrit-Reviewer: honeyhexin <[email protected]> Gerrit-Comment-Date: Sun, 28 Jul 2019 21:59:45 +0000 Gerrit-HasComments: Yes
