Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11197 )
Change subject: Add delete_external_catalogs flag to table delete tool ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/client/client.h@547 PS1, Line 547: Status KUDU_NO_EXPORT DeleteTableWithOption(const std::string& table_name, Could this just be called DeleteTable? 'WithOption' implies there would be an options struct or something similar. http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/client/client.cc@400 PS1, Line 400: return data_->DeleteTable(this, table_name, deadline); Could simplify this by forwarding to the new API. http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/master/master.proto@449 PS1, Line 449: optional bool delete_external_catalogs = 2 [default = true]; What do you think about unifying the names of 'delete_external_catalogs' and 'alter_external_catalogs', perhaps to 'modify_external_catalogs'? I think it may be easier to understand that way, since they affect the DDL ops in the same way. It would also mean only one CLI flag is needed. http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/tools/tool_action_table.cc@129 PS1, Line 129: return client->DeleteTableWithOption(table_name, FLAGS_delete_external_catalogs); Add a CLI test. -- To view, visit http://gerrit.cloudera.org:8080/11197 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 Gerrit-Change-Number: 11197 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 14 Aug 2018 20:53:50 +0000 Gerrit-HasComments: Yes
