Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13314 )
Change subject: hms: don't overwrite table type when updating metadata ...................................................................... Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/13314/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13314/2//COMMIT_MSG@9 PS2, Line 9: During the inception of Kudu's HMS integration, it was assumed that : managed/external tables would no longer be supported. I'm not sure this sounds clear w.r.t. what type of tables were slated for obsolescence: managed or external? http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/hms/hms_catalog.h@41 PS2, Line 41: MANAGED, : EXTERNAL nit: re-order to be in alphabetic order? Or there is some idea to have these in this particular order? http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/hms/hms_catalog.cc File src/kudu/hms/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/hms/hms_catalog.cc@359 PS2, Line 359: Hive will create HDFS directories when : // creating tables for Kudu I'm curious why we don't see it with our mini-hms tests. Is that because mini-hms doesn't have the same integration with other hadoop ecosystem as it's in the wild? However, I didn't see anything that would look like HDFS-related in our mini-hms configs. I guess my question here is should we try to add a test scenario for triage the mentioned artifact in systests and alike? http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/tools/kudu-tool-test.cc@3254 PS2, Line 3254: ASSERT_TRUE nit: maybe ASSERT_NE(nullptr, external_table_key) ? http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/tools/kudu-tool-test.cc@3269 PS2, Line 3269: HmsClient::kLegacyKuduTableNameKey Should we verify that the name of Kudu table matches the value of the kLegacyKuduTableNameKey property? http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/tools/kudu-tool-test.cc@3270 PS2, Line 3270: ASSERT_FALSE(ContainsKey(hms_table.parameters, HmsClient::kKuduTableIdKey)); What about the following fields: * have::Table::tableType * hive::Table::owner Is there any constraints on those to check? Also, what about HmsClient::kKuduMasterAddrsKey property in Hive table? Should it be set to master addresses values as well? http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/tools/kudu-tool-test.cc@3291 PS2, Line 3291: // Start with legacy managed and external tables. This seems non-complete. http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/tools/kudu-tool-test.cc@3313 PS2, Line 3313: "legacy_managed" nit: maybe, create a constant for this character literal and use it in this scenario? http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/tools/kudu-tool-test.cc@3317 PS2, Line 3317: legacy_external ditto http://gerrit.cloudera.org:8080/#/c/13314/2/src/kudu/tools/kudu-tool-test.cc@3641 PS2, Line 3641: vector<pair<string, string>>( nit: would it work without the explicit type here? -- To view, visit http://gerrit.cloudera.org:8080/13314 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If1b658ccb1186e4a238567b08dfbc00b0f2244f6 Gerrit-Change-Number: 13314 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 13 May 2019 17:51:43 +0000 Gerrit-HasComments: Yes
