You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/04/13 23:33:37 UTC
[kudu-CR](branch-1.12.x) [catalog manager] cache masters' addresses
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Bankim Bhavsar,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15725
to review the following change.
Change subject: [catalog manager] cache masters' addresses
......................................................................
[catalog manager] cache masters' addresses
While troubleshooting one performance issue if running a big cluster
with large number of tables and high rate of ConnectToMaster requests,
in the logs I noticed many reports like the following:
0323 03:59:31.091574 (+114857us) spinlock_profiling.cc:243]
Waited 114 ms on lock 0x4d0ee8c. stack:
0000000002398852
00000000020d45b3
0000000000a8f8fc
0000000000aa6300
000000000221aaa8
...
which translates into
(anonymous namespace)::SubmitSpinLockProfileData()
consensus::RaftConsensus::CommittedConfig()
master::Master::GetMasterHostPorts()
master::MasterServiceImpl::ConnectToMaster()
rpc::GeneratedServiceIf::Handle()
...
From the code it became apparent that the lock in question was
LockGuard l(lock_);
in RaftConsensus::CommittedConfig() accessor method.
This patch introduces caching of the master host ports, so there is
no need to fetch the information on the master addresses from the
consensus metadata every time ConnectToMaster() is called. Since the
membership for master Raft consensus is static, it's enough to fetch
the information on the masters' addresses upon catalog initialization
and use that information since then. If we implement dynamic master
change config, we'll need to refresh the cached state on change config.
This is a follow-up to 14912a1fd78ba7cf4d62bf934ae64d6f6f229ee6.
Change-Id: Ic9afea2d708bd3060b3d9f5b672660e1d2dca910
Reviewed-on: http://gerrit.cloudera.org:8080/15704
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Bankim Bhavsar <ba...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
(cherry picked from commit b2e4d952b6134a47db383fa2a0a88ac47cbaecc1)
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master_service.cc
5 files changed, 24 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/15725/1
--
To view, visit http://gerrit.cloudera.org:8080/15725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: branch-1.12.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic9afea2d708bd3060b3d9f5b672660e1d2dca910
Gerrit-Change-Number: 15725
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR](branch-1.12.x) [catalog manager] cache masters' addresses
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15725 )
Change subject: [catalog manager] cache masters' addresses
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/15725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: branch-1.12.x
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9afea2d708bd3060b3d9f5b672660e1d2dca910
Gerrit-Change-Number: 15725
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 14 Apr 2020 04:19:14 +0000
Gerrit-HasComments: No
[kudu-CR](branch-1.12.x) [catalog manager] cache masters' addresses
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15725 )
Change subject: [catalog manager] cache masters' addresses
......................................................................
[catalog manager] cache masters' addresses
While troubleshooting one performance issue if running a big cluster
with large number of tables and high rate of ConnectToMaster requests,
in the logs I noticed many reports like the following:
0323 03:59:31.091574 (+114857us) spinlock_profiling.cc:243]
Waited 114 ms on lock 0x4d0ee8c. stack:
0000000002398852
00000000020d45b3
0000000000a8f8fc
0000000000aa6300
000000000221aaa8
...
which translates into
(anonymous namespace)::SubmitSpinLockProfileData()
consensus::RaftConsensus::CommittedConfig()
master::Master::GetMasterHostPorts()
master::MasterServiceImpl::ConnectToMaster()
rpc::GeneratedServiceIf::Handle()
...
From the code it became apparent that the lock in question was
LockGuard l(lock_);
in RaftConsensus::CommittedConfig() accessor method.
This patch introduces caching of the master host ports, so there is
no need to fetch the information on the master addresses from the
consensus metadata every time ConnectToMaster() is called. Since the
membership for master Raft consensus is static, it's enough to fetch
the information on the masters' addresses upon catalog initialization
and use that information since then. If we implement dynamic master
change config, we'll need to refresh the cached state on change config.
This is a follow-up to 14912a1fd78ba7cf4d62bf934ae64d6f6f229ee6.
Change-Id: Ic9afea2d708bd3060b3d9f5b672660e1d2dca910
Reviewed-on: http://gerrit.cloudera.org:8080/15704
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Bankim Bhavsar <ba...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
(cherry picked from commit b2e4d952b6134a47db383fa2a0a88ac47cbaecc1)
Reviewed-on: http://gerrit.cloudera.org:8080/15725
Reviewed-by: Hao Hao <ha...@cloudera.com>
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master_service.cc
5 files changed, 24 insertions(+), 20 deletions(-)
Approvals:
Kudu Jenkins: Verified
Bankim Bhavsar: Looks good to me, but someone else must approve
Hao Hao: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/15725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: branch-1.12.x
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic9afea2d708bd3060b3d9f5b672660e1d2dca910
Gerrit-Change-Number: 15725
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR](branch-1.12.x) [catalog manager] cache masters' addresses
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15725 )
Change subject: [catalog manager] cache masters' addresses
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: branch-1.12.x
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9afea2d708bd3060b3d9f5b672660e1d2dca910
Gerrit-Change-Number: 15725
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 14 Apr 2020 05:33:48 +0000
Gerrit-HasComments: No