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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 06 Sep 2018 17:30:11 +0000 Gerrit-HasComments: Yes
