You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2020/09/23 03:53:48 UTC

[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 <gr...@apache.org>

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <aw...@cloudera.com>
Tested-by: Grant Henke <gr...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 24 Sep 2020 22:11:20 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Oct 2020 14:17:06 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 30 Sep 2020 16:14:07 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 30 Sep 2020 18:58:08 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Oct 2020 18:01:09 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Oct 2020 21:52:45 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Oct 2020 21:39:31 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 30 Sep 2020 22:32:36 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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(&normalized_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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 30 Sep 2020 21:23:29 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Oct 2020 22:59:04 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Sep 2020 18:35:49 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Sep 2020 13:15:56 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Oct 2020 22:53:57 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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(&client, 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<simple_spinlock> 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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Oct 2020 22:03:20 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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(&client, 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<simple_spinlock> 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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Oct 2020 22:10:09 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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(&client, 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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 23 Sep 2020 23:29:54 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Oct 2020 22:54:15 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 30 Sep 2020 22:33:06 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3192: [hms] Leverage the cluster ID in HMS sync

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 29 Sep 2020 21:54:05 +0000
Gerrit-HasComments: Yes