Dan Burkert 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. I'm not against adding that, but consider that this error will occur for every operation that hits the HMS (create table/all alter tables/drop table), not just ALTER TABLE DROP COLUMN ops. In that context I think it makes sense to just have a blanket statement 'this config is required', and not get in to too much detail. This is the same reason I didn't include the reasoning in the kudu metastore plugin check above (plus that one is way more nuanced :). 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 ta We're going to have to document this new behavior very prominently, because it will surely trip up pretty much every current Kudu user that goes to turn on the HMS integration. Given that it will hopefully be documented widely in other public facing locations, I'm not sure it's worth calling out in specific unit tests. 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? Done 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 I've added the comment, and added the 'happy' path, I was so focused on the edge cases I forgot about that! 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. Done 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 Kud Correct. 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 can't directly call this method, its called by the catalog manager, who checks that the Kudu table does exist before proceeding with an alter operation. It's important to support this usecase so that users can rename 'legacy' Kudu tables that don't have an entry in the HMS, and have that create the new entry with the corrected 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 18:57:57 +0000 Gerrit-HasComments: Yes
