Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/10817 )
Change subject: WIP: KUDU-2191: support table-name identifiers with upper case chars ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/10817/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10817/1//COMMIT_MSG@7 PS1, Line 7: with upper case chars I feel it might be a bit confusing to call it support upper case table name, since the patch is actually making table naming case insensitive as what I understand. It may be more clear to the users if we explicitly call out that table naming is now case insensitive with HMS integration enabled. http://gerrit.cloudera.org:8080/#/c/10817/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10817/2//COMMIT_MSG@47 PS2, Line 47: FOO.baz nit: FOO.bar http://gerrit.cloudera.org:8080/#/c/10817/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10817/3//COMMIT_MSG@54 PS3, Line 54: source : of confusion I feel confusion might be mitigated if we always store the normalized form, and it will be more aligned with the HMS behavior. But it looks like it is hard be do so for legacy tables. http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/hms/hms_catalog.cc File src/kudu/hms/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/hms/hms_catalog.cc@105 PS3, Line 105: '_', and '/' Is this naming restriction configurable in HMS? http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/integration-tests/master_hms-itest.cc@65 PS3, Line 65: MasterHmsTest : public ExternalMiniClusterITestBase { Should we add some more test cases with upper cases table name in other integration tests, e.g. master-stress-test. http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/integration-tests/master_hms-itest.cc@529 PS3, Line 529: cluster_->ShutdownNodes(cluster::ClusterNodes::MASTERS_ONLY); : cluster_->EnableMetastoreIntegration(); : ASSERT_TRUE(cluster_->Restart().IsNetworkError()); nit: add a quick comment for what those lines are doing. http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/master/catalog_manager.cc@1513 PS3, Line 1513: table_name When HMS integration is enabled, maybe we should log the normalized table name to avoid confusion? http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/master/catalog_manager.cc@2301 PS3, Line 2301: In this case Since without HMS integration, we do not allow rename to same table name. It feels a bit wired that we consider rename to same normalized name legal while create another table with the same normalized name illegal with HMS integration. -- 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: 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: Tue, 26 Jun 2018 23:33:19 +0000 Gerrit-HasComments: Yes
