Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10817 )
Change subject: KUDU-2191: support table-name identifiers with upper case chars ...................................................................... Patch Set 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/hms/hms_catalog.h@162 PS3, Line 162: Status Reconnect(); > But why is it important for NormalizeTableName to mutate in place? It's not inherently important, but I don't think it's more complicated to do it in-place than the alternative. http://gerrit.cloudera.org:8080/#/c/10817/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/10817/5/src/kudu/master/catalog_manager.cc@4796 PS5, Line 4796: In this case the table is guaranteed to be a legacy table which : // has survived since before the cluster was configured to integrate with : // the HMS. > That's not true across the board though; NormalizeTableName is used to norm Well in CreateTable there shouldn't be an existing table at all, so this comment doesn't really apply. If NormalizeTableName returned an error then CreateTable would have to conditionally call it based on whether the hms integration is enabled or not. I think it's simpler the way it is. If the integration is enabled and the table name isn't valid, then CreateTable will still fail because the table name gets validated again in HmsCatalog::CreateTable, and then again by the HMS. http://gerrit.cloudera.org:8080/#/c/10817/5/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/10817/5/src/kudu/mini-cluster/external_mini_cluster.cc@264 PS5, Line 264: void ExternalMiniCluster::DisableMetastoreIntegration() { > Shouldn't this enforce that the cluster is shut down? The enforcement in Se Done -- To view, visit http://gerrit.cloudera.org:8080/10817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I18977d6fe7b2999a36681a728ac0d1e54b7f38cd Gerrit-Change-Number: 10817 Gerrit-PatchSet: 7 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: Tue, 03 Jul 2018 19:52:56 +0000 Gerrit-HasComments: Yes
