[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16494 ) Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync .. KUDU-3192: [hms] Leverage the cluster ID in HMS sync This patch propagates the cluster ID to the HMS entries for Kudu tables and leverages the cluster ID to ignore irrelevant notifications from the HMS. Additionally the `kudu hms` tool is updated to identify and repair tables that do not have the cluster ID set. Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Reviewed-on: http://gerrit.cloudera.org:8080/16494 Reviewed-by: Andrew Wong Tested-by: Grant Henke Reviewed-by: Alexey Serbin --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/integration-tests/hms_itest-base.cc M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/hms_notification_log_listener.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master_service.cc M src/kudu/master/sys_catalog-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 17 files changed, 264 insertions(+), 126 deletions(-) Approvals: Andrew Wong: Looks good to me, approved Grant Henke: Verified Alexey Serbin: Looks good to me, approved -- 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: merged Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 11 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Alexey Serbin 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 10: Code-Review+2 -- 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: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Oct 2020 22:59:04 + Gerrit-HasComments: No
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Grant Henke has removed a vote on this change. Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync .. Removed Verified-1 by Kudu Jenkins (120) -- 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: deleteVote Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
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 10: Verified+1 -- 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: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Oct 2020 22:54:15 + Gerrit-HasComments: No
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Andrew Wong 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 10: Code-Review+2 -- 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: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Oct 2020 22:53:57 + Gerrit-HasComments: No
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16494 to look at the new patch set (#10). Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync .. KUDU-3192: [hms] Leverage the cluster ID in HMS sync This patch propagates the cluster ID to the HMS entries for Kudu tables and leverages the cluster ID to ignore irrelevant notifications from the HMS. Additionally the `kudu hms` tool is updated to identify and repair tables that do not have the cluster ID set. Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/integration-tests/hms_itest-base.cc M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/hms_notification_log_listener.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master_service.cc M src/kudu/master/sys_catalog-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 17 files changed, 264 insertions(+), 126 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/16494/10 -- 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: newpatchset Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
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 9: (2 comments) 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@169 PS1, Line 169: ASSERT_TRUE(CreateTable(, database_name, table_name, : table_id, cluster_id).IsAlreadyPresent()); > Thank you for the clarification. That is correct. http://gerrit.cloudera.org:8080/#/c/16494/9/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16494/9/src/kudu/master/catalog_manager.cc@1045 PS9, Line 1045: std::lock_guard l(cluster_id_lock_); > I'm not sure I understand why this lock is necessary here. As I can see, ' Good catch, I was doing a find and replace too quickly. -- 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: 9 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Oct 2020 22:10:09 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Alexey Serbin 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 9: (2 comments) 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@169 PS1, Line 169: ASSERT_TRUE(CreateTable(, database_name, table_name, : table_id, cluster_id).IsAlreadyPresent()); > CreateTable in the HMS client is HMS specific. Only the table_name matters Thank you for the clarification. Just wanted to make sure my understanding is correct w.r.t. table name being the primary identifier for a table in HMS. Does it mean it's not possible to have two tables with same name in different clusters managed by the same HMS instance? http://gerrit.cloudera.org:8080/#/c/16494/9/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16494/9/src/kudu/master/catalog_manager.cc@1045 PS9, Line 1045: std::lock_guard l(cluster_id_lock_); I'm not sure I understand why this lock is necessary here. As I can see, 'cluster_id' is an output parameter and should not be shared among multiple threads calling this method, and 'entry' is on stack of every thread that would call this method, right? -- 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: 9 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Oct 2020 22:03:20 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Andrew Wong 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 9: Code-Review+2 -- 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: 9 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Oct 2020 21:52:45 + Gerrit-HasComments: No
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16494 to look at the new patch set (#9). Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync .. KUDU-3192: [hms] Leverage the cluster ID in HMS sync This patch propagates the cluster ID to the HMS entries for Kudu tables and leverages the cluster ID to ignore irrelevant notifications from the HMS. Additionally the `kudu hms` tool is updated to identify and repair tables that do not have the cluster ID set. Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/integration-tests/hms_itest-base.cc M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/hms_notification_log_listener.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master_service.cc M src/kudu/master/sys_catalog-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 17 files changed, 269 insertions(+), 128 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/16494/9 -- 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: newpatchset Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 9 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
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 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/16494/8/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16494/8/src/kudu/master/catalog_manager.cc@4608 PS8, Line 4608: leader_lock_.AssertAcquiredForReading(); > Doesn't this mean that only leaders can return the cluster ID? If so, what' The leader_lock is also used by followers when reading data, I think it's more of a leadership_change_lock. Regardless it's much more straightforward to use a separate lock for the cluster ID. -- 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: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Oct 2020 21:39:31 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Andrew Wong 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 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/16494/8/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16494/8/src/kudu/master/catalog_manager.cc@4608 PS8, Line 4608: leader_lock_.AssertAcquiredForReading(); Doesn't this mean that only leaders can return the cluster ID? If so, what's the point of PrepareFollowerClusterId()? Alternatively we could have a separate lock for it, or maybe re-use lock_ assuming that's safe. -- 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: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Oct 2020 18:01:09 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16494 to look at the new patch set (#8). Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync .. KUDU-3192: [hms] Leverage the cluster ID in HMS sync This patch propagates the cluster ID to the HMS entries for Kudu tables and leverages the cluster ID to ignore irrelevant notifications from the HMS. Additionally the `kudu hms` tool is updated to identify and repair tables that do not have the cluster ID set. Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/integration-tests/hms_itest-base.cc M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/hms_notification_log_listener.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master_service.cc M src/kudu/master/sys_catalog-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 17 files changed, 267 insertions(+), 128 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/16494/8 -- 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: newpatchset Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16494 to look at the new patch set (#7). Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync .. KUDU-3192: [hms] Leverage the cluster ID in HMS sync This patch propagates the cluster ID to the HMS entries for Kudu tables and leverages the cluster ID to ignore irrelevant notifications from the HMS. Additionally the `kudu hms` tool is updated to identify and repair tables that do not have the cluster ID set. Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/integration-tests/hms_itest-base.cc M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/hms_notification_log_listener.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master_service.cc M src/kudu/master/sys_catalog-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 17 files changed, 267 insertions(+), 128 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/16494/7 -- 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: newpatchset Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
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 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/16494/6/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/16494/6/src/kudu/master/catalog_manager.h@718 PS6, Line 718: const std::string& GetClusterId(); > Can this be const? Done http://gerrit.cloudera.org:8080/#/c/16494/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16494/6/src/kudu/master/catalog_manager.cc@4607 PS6, Line 4607: const string& > It's probably safer to just return a copy. It seems possible that we'd run 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: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Oct 2020 14:17:06 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Andrew Wong 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 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/16494/6/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/16494/6/src/kudu/master/catalog_manager.h@718 PS6, Line 718: const std::string& GetClusterId(); Can this be const? -- 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: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 30 Sep 2020 22:33:06 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Andrew Wong 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 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/16494/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16494/6/src/kudu/master/catalog_manager.cc@4607 PS6, Line 4607: const string& It's probably safer to just return a copy. It seems possible that we'd run into a race like: T1: lock for reading T1: return a reference to the cluster ID T1: unlock for reading T2: lock and update cluster ID (e.g. first time starting up and becoming leader) T1: try to use the cluster ID reference, but race with T2 -- 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: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 30 Sep 2020 22:32:36 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16494 to look at the new patch set (#6). Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync .. KUDU-3192: [hms] Leverage the cluster ID in HMS sync This patch propagates the cluster ID to the HMS entries for Kudu tables and leverages the cluster ID to ignore irrelevant notifications from the HMS. Additionally the `kudu hms` tool is updated to identify and repair tables that do not have the cluster ID set. Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/integration-tests/hms_itest-base.cc M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/hms_notification_log_listener.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master_service.cc M src/kudu/master/sys_catalog-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 17 files changed, 260 insertions(+), 126 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/16494/6 -- 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: newpatchset Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
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 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/16494/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16494/4/src/kudu/master/catalog_manager.cc@5511 PS4, Line 5511: // names when normalized. : ignore_result(hms::HmsCatalog::NormalizeTableName(_table_name)); : } : > Looking around for how this can be thread-safe, I think the answer is that This actually manifested in some TSAN failures. I moved the cluster ID back into the CatalogManager and exposed it via `GetClusterId` which calls `CatalogManager::GetClusterId`. -- 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: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 30 Sep 2020 21:23:29 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16494 to look at the new patch set (#5). Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync .. KUDU-3192: [hms] Leverage the cluster ID in HMS sync This patch propagates the cluster ID to the HMS entries for Kudu tables and leverages the cluster ID to ignore irrelevant notifications from the HMS. Additionally the `kudu hms` tool is updated to identify and repair tables that do not have the cluster ID set. Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/integration-tests/hms_itest-base.cc M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/hms_notification_log_listener.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master_service.cc M src/kudu/master/sys_catalog-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 17 files changed, 259 insertions(+), 125 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/16494/5 -- 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: newpatchset Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Andrew Wong 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 4: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16494/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16494/4/src/kudu/master/catalog_manager.cc@5511
PS4, Line 5511:
: const string& CatalogManager::cluster_id() const {
: return master_->cluster_id();
: }
Looking around for how this can be thread-safe, I think the answer is that this
is only mutated and accessed while the leader lock is held. If so, can you add
a comment mentioning that? And maybe assert that a lock is held?
--
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: 4
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 30 Sep 2020 18:58:08 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16494 to look at the new patch set (#4). Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync .. KUDU-3192: [hms] Leverage the cluster ID in HMS sync This patch propagates the cluster ID to the HMS entries for Kudu tables and leverages the cluster ID to ignore irrelevant notifications from the HMS. Additionally the `kudu hms` tool is updated to identify and repair tables that do not have the cluster ID set. Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/integration-tests/hms_itest-base.cc M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/hms_notification_log_listener.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 13 files changed, 248 insertions(+), 110 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/16494/4 -- 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: newpatchset Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
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: (2 comments) http://gerrit.cloudera.org:8080/#/c/16494/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16494/1//COMMIT_MSG@9 PS1, Line 9: This patch propagates the cluster ID to the HMS entries for > Could you also add tests where we create some real-looking metadata with no 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@437 PS1, Line 437: // If there is not a cluster ID, for maximum compatibility we should assume this is an older > Uncomment this? Also maybe encapsulate in some FilterDifferentClusterId() m I thought about adding a HasDifferentClusterId method, but doesn't save anything because I still need to log and return in each method and the log needs the cluster_id. -- 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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 30 Sep 2020 16:14:07 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16494 to look at the new patch set (#3). Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync .. KUDU-3192: [hms] Leverage the cluster ID in HMS sync This patch propagates the cluster ID to the HMS entries for Kudu tables and leverages the cluster ID to ignore irrelevant notifications from the HMS. Additionally the `kudu hms` tool is updated to identify and repair tables that do not have the cluster ID set. Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/integration-tests/hms_itest-base.cc M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/hms_notification_log_listener.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 13 files changed, 247 insertions(+), 110 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/16494/3 -- 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: newpatchset Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
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(, 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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 29 Sep 2020 21:54:05 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16494 to look at the new patch set (#2). Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync .. KUDU-3192: [hms] Leverage the cluster ID in HMS sync This patch propagates the cluster ID to the HMS entries for Kudu tables and leverages the cluster ID to ignore irrelevant notifications from the HMS. Additionally the `kudu hms` tool is updated to identify and repair tables that do not have the cluster ID set. Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/integration-tests/hms_itest-base.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/hms_notification_log_listener.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 12 files changed, 185 insertions(+), 110 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/16494/2 -- 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: newpatchset Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Hao Hao 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: (4 comments) 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 methods. 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 clusters? 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. 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. -- 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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 24 Sep 2020 22:11:20 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Alexey Serbin 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: (2 comments) 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@169 PS1, Line 169: ASSERT_TRUE(CreateTable(, database_name, table_name, : table_id, cluster_id).IsAlreadyPresent()); What if supplying the same table_id, but different cluster_id? What should be the behavior? I guess it should report Status::AlreadyPresent, and I think it would be great to explicitly document the expected behavior in this scenario. 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 along with others once cluster_id is introduced, no? If so, probably it make sense to make sure such property is present in the response? -- 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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Sep 2020 23:29:54 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Andrew Wong 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: (3 comments) http://gerrit.cloudera.org:8080/#/c/16494/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16494/1//COMMIT_MSG@9 PS1, Line 9: This patch propagates the cluster ID to the HMS entries for Could you also add tests where we create some real-looking metadata with no or different cluster IDs and ensure we ignore or account for it as appropriate? 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@363 PS1, Line 363: before_table.tableName, cluster_id); Should dereference cluster_id http://gerrit.cloudera.org:8080/#/c/16494/1/src/kudu/master/hms_notification_log_listener.cc@437 PS1, Line 437: // If there is not a cluster ID, for maximum compatibility we should assume this is an older Uncomment this? Also maybe encapsulate in some FilterDifferentClusterId() method? -- 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 Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Sep 2020 18:35:49 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
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: Verified+1 -- 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 Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Sep 2020 13:15:56 + Gerrit-HasComments: No
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Grant Henke has removed a vote on this change. Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync .. Removed Verified-1 by Kudu Jenkins (120) -- 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: deleteVote Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16494 Change subject: KUDU-3192: [hms] Leverage the cluster ID in HMS sync .. KUDU-3192: [hms] Leverage the cluster ID in HMS sync This patch propagates the cluster ID to the HMS entries for Kudu tables and leverages the cluster ID to ignore irrelevant notifications from the HMS. Additionally the `kudu hms` tool is updated to identify and repair tables that do not have the cluster ID set. Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/hms_client.h M src/kudu/integration-tests/hms_itest-base.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/hms_notification_log_listener.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 11 files changed, 174 insertions(+), 108 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/16494/1 -- 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: newchange Gerrit-Change-Id: I865b418a3cc4e11c889cc4757cd940831c43af17 Gerrit-Change-Number: 16494 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
