Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16494 )
Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync ...................................................................... Patch Set 1: (8 comments) I am working on adding more tests, but pushing my rebased updates for now. http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/hms/hms_catalog.h@66 PS1, Line 66: // Creates a new table entry in the HMS. : // : // If 'owner' is omitted the table will be created without an owner. This is : // useful in circumstances where the owner is not known, for example when : // creating an HMS table entry for an existing Kudu table. : // : // Fails the HMS is unreachable, or a table with the same name is already present. > nit: update the comment with the cluster_id? The same for other similar met This is internal API and doesn't mention all the other fields. I imagine the docs would be fairly self evident. http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/hms/hms_client-test.cc File src/kudu/hms/hms_client-test.cc: http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/hms/hms_client-test.cc@60 PS1, Line 60: Status CreateTable(HmsClient* client, > warning: method 'CreateTable' can be made static [readability-convert-membe Done http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/hms/hms_client-test.cc@169 PS1, Line 169: ASSERT_TRUE(CreateTable(&client, database_name, table_name, : table_id, cluster_id).IsAlreadyPresent()); > What if supplying the same table_id, but different cluster_id? What should CreateTable in the HMS client is HMS specific. Only the table_name matters for an already present table. This is the same behavior as all other fields and defined by the HMS itself. http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/hms/hms_client-test.cc@179 PS1, Line 179: EXPECT_EQ(HmsClient::kManagedTable, my_table.tableType); > I guess GetTable() should return HmsClient::kKuduClusterIdKey property alon Done http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/master/hms_notification_log_listener.cc File src/kudu/master/hms_notification_log_listener.cc: http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/master/hms_notification_log_listener.cc@356 PS1, Line 356: This is safe because we still validate the table ID : // which is universally unique > Does it mean the table ID will never be the same even in across different c Right, the ID is a UUID. http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/master/hms_notification_log_listener.cc@363 PS1, Line 363: before_table.tableName, cluster_id); > Should dereference cluster_id Done http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/tools/tool_action_hms.cc@245 PS1, Line 245: row.resize > nit: samer here. Done http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/tools/tool_action_hms.cc@256 PS1, Line 256: adjust row.resize below > nit: adjust 'row.resize' below. Done -- To view, visit http://gerrit.cloudera.org:8080/16494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 29 Sep 2020 21:54:05 +0000 Gerrit-HasComments: Yes
