Dan Burkert 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) 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? It's not necessary because it's already enforced in AlterTable, and if !alter_external_catalogs then AlterTable is called directly (as opposed to indirectly through the notification log listener). 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 ; Will address in https://gerrit.cloudera.org/c/10817/ 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 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? This is a reasonable line of inquiry, and I agree with the conclusion that it _is_ necessary even if KUDU-2475 is addressed. > So why does the existence of KUDU-2475 force us to duplicate the check here? > What am I missing? I'm not sure I understand the question - as you mentioned it's necessary to do the check ahead of time to ensure that the HMS isn't altered. bigger perspective: Kudu maintains a constraint on tables that the HMS doesn't. When the HMS integration is enabled the catalog manager cedes responsibility for maintaining the no-duplicate-table-name invariant to the HMS, but this is a special case that the HMS doesn't validate, so the master has to do it itself. -- 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:56:17 +0000 Gerrit-HasComments: Yes
