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 2019/03/13 23:20:11 UTC
[kudu-CR] [TSManager] don't hold lock while running LA command
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12749
Change subject: [TSManager] don't hold lock while running LA command
......................................................................
[TSManager] don't hold lock while running LA command
This patch modifies the way how the location assignment is run
during the course of tablet server registration with master.
Prior to this patch, TSManager's lock on the registry of all
tablet servers was held while running the location assignment
command. That might lead to stacking many TS registration
heartbeats in flight while master's service threads are waiting
to acquire the TSManager's lock.
This patch introduces changes to run the location assignment command
without holding the TSManager's lock, which makes the concurrent
registration of tablet servers more robust and make the processing
of tablet servers' heartbeats more robust overall.
Change-Id: I59cd5f6ed19c162a7c9f9a6527e78cab782b4539
---
M src/kudu/master/location_cache-test.cc
M src/kudu/master/master.cc
M src/kudu/master/ts_descriptor-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M src/kudu/master/ts_manager.cc
6 files changed, 123 insertions(+), 141 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/12749/1
--
To view, visit http://gerrit.cloudera.org:8080/12749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I59cd5f6ed19c162a7c9f9a6527e78cab782b4539
Gerrit-Change-Number: 12749
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [TSManager] don't hold lock while running LA command
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12749 )
Change subject: [TSManager] don't hold lock while running LA command
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12749/1/src/kudu/master/ts_manager.cc
File src/kudu/master/ts_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12749/1/src/kudu/master/ts_manager.cc@143
PS1, Line 143: location.emplace(location_str);
No std::move()?
http://gerrit.cloudera.org:8080/#/c/12749/1/src/kudu/master/ts_manager.cc@150
PS1, Line 150: if (ContainsKey(servers_by_id_, uuid)) {
: descriptor = FindOrDie(servers_by_id_, uuid);
Should be able to use a Find* function to avoid the double lookup in the reregistration case.
--
To view, visit http://gerrit.cloudera.org:8080/12749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59cd5f6ed19c162a7c9f9a6527e78cab782b4539
Gerrit-Change-Number: 12749
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 00:41:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TSManager] don't hold lock while running LA command
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12749 )
Change subject: [TSManager] don't hold lock while running LA command
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/12749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59cd5f6ed19c162a7c9f9a6527e78cab782b4539
Gerrit-Change-Number: 12749
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 21:06:17 +0000
Gerrit-HasComments: No
[kudu-CR] [TSManager] don't hold lock while running LA command
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12749 )
Change subject: [TSManager] don't hold lock while running LA command
......................................................................
[TSManager] don't hold lock while running LA command
This patch modifies the way how the location assignment is run
during the course of tablet server registration with master.
Prior to this patch, TSManager's lock on the registry of all
tablet servers was held while running the location assignment
command. That might lead to stacking many TS registration
heartbeats in flight while master's service threads are waiting
to acquire the TSManager's lock.
This patch introduces changes to run the location assignment command
without holding the TSManager's lock. That makes the registration
of tablet servers with masters more robust and makes the processing
of concurrent heartbeats from tablet servers more efficient overall.
Change-Id: I59cd5f6ed19c162a7c9f9a6527e78cab782b4539
Reviewed-on: http://gerrit.cloudera.org:8080/12749
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley <wd...@gmail.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/master/location_cache-test.cc
M src/kudu/master/master.cc
M src/kudu/master/ts_descriptor-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M src/kudu/master/ts_manager.cc
6 files changed, 124 insertions(+), 143 deletions(-)
Approvals:
Kudu Jenkins: Verified
Will Berkeley: Looks good to me, approved
Adar Dembo: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/12749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I59cd5f6ed19c162a7c9f9a6527e78cab782b4539
Gerrit-Change-Number: 12749
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [TSManager] don't hold lock while running LA command
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12749
to look at the new patch set (#2).
Change subject: [TSManager] don't hold lock while running LA command
......................................................................
[TSManager] don't hold lock while running LA command
This patch modifies the way how the location assignment is run
during the course of tablet server registration with master.
Prior to this patch, TSManager's lock on the registry of all
tablet servers was held while running the location assignment
command. That might lead to stacking many TS registration
heartbeats in flight while master's service threads are waiting
to acquire the TSManager's lock.
This patch introduces changes to run the location assignment command
without holding the TSManager's lock. That makes the registration
of tablet servers with masters more robust and makes the processing
of concurrent heartbeats from tablet servers more efficient overall.
Change-Id: I59cd5f6ed19c162a7c9f9a6527e78cab782b4539
---
M src/kudu/master/location_cache-test.cc
M src/kudu/master/master.cc
M src/kudu/master/ts_descriptor-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M src/kudu/master/ts_manager.cc
6 files changed, 124 insertions(+), 143 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/12749/2
--
To view, visit http://gerrit.cloudera.org:8080/12749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I59cd5f6ed19c162a7c9f9a6527e78cab782b4539
Gerrit-Change-Number: 12749
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [TSManager] don't hold lock while running LA command
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12749 )
Change subject: [TSManager] don't hold lock while running LA command
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59cd5f6ed19c162a7c9f9a6527e78cab782b4539
Gerrit-Change-Number: 12749
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 18:42:42 +0000
Gerrit-HasComments: No
[kudu-CR] [TSManager] don't hold lock while running LA command
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12749 )
Change subject: [TSManager] don't hold lock while running LA command
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12749/1/src/kudu/master/ts_manager.cc
File src/kudu/master/ts_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12749/1/src/kudu/master/ts_manager.cc@61
PS1, Line 61: using std::vector;
> warning: using decl 'vector' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/12749/1/src/kudu/master/ts_manager.cc@143
PS1, Line 143: location.emplace(location_str);
> No std::move()?
Done
http://gerrit.cloudera.org:8080/#/c/12749/1/src/kudu/master/ts_manager.cc@150
PS1, Line 150: if (ContainsKey(servers_by_id_, uuid)) {
: descriptor = FindOrDie(servers_by_id_, uuid);
> Should be able to use a Find* function to avoid the double lookup in the re
Whoops, I thought the same but then missed updating this. Done.
--
To view, visit http://gerrit.cloudera.org:8080/12749
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59cd5f6ed19c162a7c9f9a6527e78cab782b4539
Gerrit-Change-Number: 12749
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 02:05:47 +0000
Gerrit-HasComments: Yes