Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8312 )
Change subject: KUDU-2191 (6/n): Hive Metastore catalog manager integration ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/8312/5/src/kudu/hms/hms_client.cc File src/kudu/hms/hms_client.cc: http://gerrit.cloudera.org:8080/#/c/8312/5/src/kudu/hms/hms_client.cc@204 PS5, Line 204: "Hive Metastore configuration is invalid: $0 must be set to false", Add "to support dropping columns" to specify the reason. http://gerrit.cloudera.org:8080/#/c/8312/5/src/kudu/integration-tests/alter_table-randomized-test.cc File src/kudu/integration-tests/alter_table-randomized-test.cc: http://gerrit.cloudera.org:8080/#/c/8312/5/src/kudu/integration-tests/alter_table-randomized-test.cc@79 PS5, Line 79: const char* kTableName = "default.test_table"; Do we need to add a comment that when enable_hive_metastore is true, the table name has to have databasename.tablename pattern? http://gerrit.cloudera.org:8080/#/c/8312/6/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/8312/6/src/kudu/integration-tests/master_hms-itest.cc@176 PS6, Line 176: ASSERT_EQ(table->id(), hms_table.parameters[hms::HmsClient::kKuduTableIdKey]); Assert master addresses as well? http://gerrit.cloudera.org:8080/#/c/8312/6/src/kudu/integration-tests/master_hms-itest.cc@236 PS6, Line 236: // Drop the HMS table entry and rename the table. I do not quite follow why do we need to drop the hms table entry? Will the drop table in HMS also trigger drop table in Kudu? If you are testing the corner case when the entry is not present in HMS, maybe add more comment here to be more clear. Can we have a most common rename table test case without any corner cases? http://gerrit.cloudera.org:8080/#/c/8312/6/src/kudu/integration-tests/master_hms-itest.cc@245 PS6, Line 245: ASSERT_OK(table_alterer->RenameTo(renamed_table_name)->Alter()); Assert table id/master addresses/table schema. http://gerrit.cloudera.org:8080/#/c/8312/6/src/kudu/integration-tests/master_hms-itest.cc@285 PS6, Line 285: ASSERT_OK(hms_client_->AlterTable(hms_database_name, hms_table_name, hms_table)); So alter column through HMS would not affect the table schema stored in Kudu? http://gerrit.cloudera.org:8080/#/c/8312/7/src/kudu/master/hms_catalog.cc File src/kudu/master/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/8312/7/src/kudu/master/hms_catalog.cc@164 PS7, Line 164: create it in the HMS I am not sure it is good to create a HMS entry if not present. What if the users make some mistakes when specifying the original table name? -- To view, visit http://gerrit.cloudera.org:8080/8312 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1b8f360408b05d6ac4f7359a8ac0dd889b196baf Gerrit-Change-Number: 8312 Gerrit-PatchSet: 7 Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 08 Mar 2018 06:40:50 +0000 Gerrit-HasComments: Yes
