Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10831 )
Change subject: KUDU-2191: reject alter table rename to same table name ...................................................................... Patch Set 3: (3 comments) I was late to the party. http://gerrit.cloudera.org:8080/#/c/10831/3/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/10831/3/src/kudu/master/catalog_manager.cc@2145 PS3, Line 2145: if (hms_catalog_ && req.has_new_table_name() && req.alter_external_catalogs()) { Should we enforce this even if !alter_external_catalogs? http://gerrit.cloudera.org:8080/#/c/10831/3/src/kudu/master/catalog_manager.cc@2152 PS3, Line 2152: // The HMS allows renaming a table to the same name (ALTER TABLE t RENAME TO t;), Nit: I think this is readable enough without the trailing ; http://gerrit.cloudera.org:8080/#/c/10831/3/src/kudu/master/catalog_manager.cc@2153 PS3, Line 2153: This : // invariant is also checked in CatalogManager::AlterTable, but when the HMS : // integration is enabled that check does not bubble up to the client (it's : // called by the notification log listener). I think I'm missing something about this part of the comment. When I first read this, I thought "interesting, does that mean this patch should be reverted when KUDU-2475 is addressed?" But it can't be reverted, because by the time the check in AlterTable runs, the HMS has already performed "ALTER TABLE t RENAME TO t", and ideally we would have prevented that from happening, right? So why does the existence of KUDU-2475 force us to duplicate the check here? What am I missing? -- To view, visit http://gerrit.cloudera.org:8080/10831 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I37d895bcc98cbc45a27ff25e97a25436273d79b0 Gerrit-Change-Number: 10831 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 27 Jun 2018 22:14:06 +0000 Gerrit-HasComments: Yes
