[kudu-CR] Add delete external catalogs flag to table delete tool
Adar Dembo 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 5: Code-Review+2 -- 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: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 06 Sep 2018 22:19:44 + Gerrit-HasComments: No
[kudu-CR] Add delete external catalogs flag to table delete tool
Hao Hao 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 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/11197/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11197/4//COMMIT_MSG@9 PS4, Line 9: The 'hms fix' tool helps recover from metadata inconsistencies between > Maybe rewrite this description as: Done http://gerrit.cloudera.org:8080/#/c/11197/4/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/11197/4/src/kudu/client/client.h@354 PS4, Line 354: Status KUDU_NO_EXPORT DeleteTableInCatalogs(const std::string& table_name, > According to the google style guide, we should only use function overloads Done http://gerrit.cloudera.org:8080/#/c/11197/4/src/kudu/client/client.h@1233 PS4, Line 1233: // Whether to apply the alteration to external catalogs, such as the Hive > I mentioned this to Dan in an earlier review, but I'd like to see this func Thanks a lot for bringing this up and I will take a note. -- 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: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 06 Sep 2018 22:18:02 + Gerrit-HasComments: Yes
[kudu-CR] Add delete external catalogs flag to table delete tool
Hello Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11197 to look at the new patch set (#5). Change subject: Add delete_external_catalogs flag to table delete tool .. Add delete_external_catalogs flag to table delete tool The 'hms fix' tool helps recover from metadata inconsistencies between Kudu and the HMS. However, there might be some unforeseen edge cases that the tool doesn't cover. To that end, this patch introduces a flag to control the deletion of tables in the Kudu catalog independently of HMS' catalog. Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc M src/kudu/tools/tool_action_table.cc 14 files changed, 79 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/11197/5 -- 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: newpatchset Gerrit-Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 Gerrit-Change-Number: 11197 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add delete external catalogs flag to table delete tool
Adar Dembo 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 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/11197/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11197/4//COMMIT_MSG@9 PS4, Line 9: Despite of 'hms fix' tool which intends to recover from metadata Maybe rewrite this description as: The 'hms fix' tool helps recover from metadata inconsistencies between Kudu and the HMS. However, there might be some unforeseen edge cases that the tool doesn't cover. To that end, this patch introduces a flag to control the deletion of tables in the Kudu catalog independently of HMS' catalog. http://gerrit.cloudera.org:8080/#/c/11197/4/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/11197/4/src/kudu/client/client.h@354 PS4, Line 354: Status KUDU_NO_EXPORT DeleteTable(const std::string& table_name, According to the google style guide, we should only use function overloads if the various overloads are semantically equivalent. I don't think this meets those guidelines: it's not like the argument set represent an alternative to const std::string&. Moreover, using the same name means it's easy to accidentally call the private API and get a somewhat inscrutable link error. https://google.github.io/styleguide/cppguide.html#Function_Overloading http://gerrit.cloudera.org:8080/#/c/11197/4/src/kudu/client/client.h@1233 PS4, Line 1233: // Whether to apply the alteration to external catalogs, such as the Hive I mentioned this to Dan in an earlier review, but I'd like to see this function made public and KUDU_NO_EXPORT. I think that's the cleanest way for us to provide private client APIs. Shouldn't happen in this patch, but something to keep in mind. -- 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: 4 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 06 Sep 2018 17:30:11 + Gerrit-HasComments: Yes
[kudu-CR] Add delete external catalogs flag to table delete tool
Hello Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11197 to look at the new patch set (#4). Change subject: Add delete_external_catalogs flag to table delete tool .. Add delete_external_catalogs flag to table delete tool Despite of 'hms fix' tool which intends to recover from metadata inconsistency between Kudu and the HMS, it is a good idea to introduce a flag to control the table deletion in the Kudu catalog independently of the HMS, to repair tables when HMS integration is enabled. Since there might be some edge cases we do not foresee and hence not covered in 'hms fix' tool. Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc M src/kudu/tools/tool_action_table.cc 14 files changed, 79 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/11197/4 -- 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: newpatchset Gerrit-Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 Gerrit-Change-Number: 11197 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add delete external catalogs flag to table delete tool
Hao Hao 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 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11197/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11197/1//COMMIT_MSG@9
PS1, Line 9: o recover from metadata
: inconsistency between Kudu and the HMS, it is a good idea to
introduce
: a flag to control the tab
> Not really understanding this: this commit introduces this flag, so how cou
Sorry for the confusion. We have 'hms fix' tool which hopefully can cover all
the cases when metadata between Kudu and the HMS become unsynchronized. But
there might be some corner cases we do not foresee, that is why I introduce
this commit. I will revise the commit message to better capture this.
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@539
PS1, Line 539: /// the necessary credentials to authenticate to the cluster,
as well as to
> Can you move this so it's next to DeleteTable?
Done
http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/client/client.h@547
PS1, Line 547: /// The resulting binary authentication credentials.
> Could this just be called DeleteTable? 'WithOption' implies there would be
Done
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: Status KuduClient::DeleteTable(const string& table_name) {
> Could simplify this by forwarding to the new API.
Done
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 modify_external_catalogs = 2 [default = true];
> What do you think about unifying the names of 'delete_external_catalogs' an
Done
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@39
PS1, Line 39:
: DECLARE_string(tables);
: DEFINE_bool(modify_external_catalogs, true,
: "Whether to modify external catalogs, such as the
Hive Metastore, "
: "when renaming a table.");
: DEFINE_bool(list_tablets, false,
> These only exist to facilitate "upgrade" workflows that take you from no HM
Not really, these can be used in normal workflows by admin to recover a cluster
when somehow metadata in Kudu and the HMS go out of sync.
http://gerrit.cloudera.org:8080/#/c/11197/1/src/kudu/tools/tool_action_table.cc@129
PS1, Line 129: RETURN_NOT_OK(CreateKuduClient(context, ));
> Add a CLI test.
Done
--
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: 3
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 06 Sep 2018 01:17:20 +
Gerrit-HasComments: Yes
[kudu-CR] Add delete external catalogs flag to table delete tool
Hello Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11197 to look at the new patch set (#3). Change subject: Add delete_external_catalogs flag to table delete tool .. Add delete_external_catalogs flag to table delete tool Despite of 'hms fix' tool which intends to recover from metadata inconsistency between Kudu and the HMS, it is a good idea to introduce a flag to control the table deletion in the Kudu catalog independently of the HMS, to repair tables when HMS integration is enabled. Since there might be some edge cases we do not foresee and hence not covered in 'hms fix' tool. Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc M src/kudu/tools/tool_action_table.cc 14 files changed, 76 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/11197/3 -- 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: newpatchset Gerrit-Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 Gerrit-Change-Number: 11197 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add delete external catalogs flag to table delete tool
Hello Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11197 to look at the new patch set (#2). Change subject: Add delete_external_catalogs flag to table delete tool .. Add delete_external_catalogs flag to table delete tool Even with 'hms fix' tool which intendd to recover from metadata inconsistency between Kudu and the HMS, it is a good idea to introduce a flag to control the table deletion in the Kudu catalog independently of the HMS to repair tables when HMS integration is enabled. Since there might be some edge cases wo do not foresee and hence not covered in 'hms fix' tool Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc M src/kudu/tools/tool_action_table.cc 14 files changed, 76 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/11197/2 -- 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: newpatchset Gerrit-Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 Gerrit-Change-Number: 11197 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add delete external catalogs flag to table delete tool
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 14 Aug 2018 20:53:50 + Gerrit-HasComments: Yes
[kudu-CR] Add delete external catalogs flag to table delete tool
Adar Dembo 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: (3 comments) http://gerrit.cloudera.org:8080/#/c/11197/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11197/1//COMMIT_MSG@9 PS1, Line 9: it might be necessary to : introduce a flag to control the table deletion in the Kudu catalog : independently of the HMS. Not really understanding this: this commit introduces this flag, so how could it "might be necessary" to introduce it in the future? 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@539 PS1, Line 539: /// Delete/drop a table with option. Can you move this so it's next to DeleteTable? 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@39 PS1, Line 39: DEFINE_bool(alter_external_catalogs, true, : "Whether to alter external catalogs, such as the Hive Metastore, " : "when renaming a table."); : DEFINE_bool(delete_external_catalogs, true, : "Whether to delete external catalogs, such as the Hive Metastore, " : "when deleting a table."); These only exist to facilitate "upgrade" workflows that take you from no HMS integration to HMS integration, right? If so, perhaps we should warn users against changing the values of these parameters? -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Mon, 13 Aug 2018 22:33:46 + Gerrit-HasComments: Yes
[kudu-CR] Add delete external catalogs flag to table delete tool
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11197 Change subject: Add delete_external_catalogs flag to table delete tool .. Add delete_external_catalogs flag to table delete tool Similar to alter_external_catalogs flag, it might be necessary to introduce a flag to control the table deletion in the Kudu catalog independently of the HMS. So we can repair tables if needed when the HMS integration is enabled. Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto M src/kudu/tools/tool_action_table.cc 8 files changed, 50 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/11197/1 -- 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: newchange Gerrit-Change-Id: I0a128fb53c974a5c839786204d56408681b434e8 Gerrit-Change-Number: 11197 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao
