You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2016/09/30 15:58:24 UTC

[kudu-CR] rpc: Add min / max negotiation threads 

Mike Percy has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4574

Change subject: rpc: Add min / max negotiation threads
......................................................................

rpc: Add min / max negotiation threads

Since we use a thread pool for connection negotiation, it would be
helpful for users to be able to specify the minimum and maximum number
of threads in that pool. Prior to this patch, there were no gflags to
control these parameters, and the only builder parameter was the maximum
(however it was not labelled as such).

This allows tuning of the per-tserver negotiation pool size, both
minimum and maximum threads to allow in the thread pool.

Change-Id: Ife98b39d5f3a340702151ab27dc8026c8bac12ac
---
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/server/server_base.cc
5 files changed, 30 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/4574/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4574
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ife98b39d5f3a340702151ab27dc8026c8bac12ac
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>

[kudu-CR] rpc: Add min / max negotiation threads

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.

Change subject: rpc: Add min / max negotiation threads
......................................................................


rpc: Add min / max negotiation threads

Since we use a thread pool for connection negotiation, it would be
helpful for users to be able to specify the minimum and maximum number
of threads in that pool. Prior to this patch, there were no gflags to
control these parameters, and the only builder parameter was the maximum
(however it was not labelled as such).

This allows tuning of the per-tserver negotiation pool size, both
minimum and maximum threads to allow in the thread pool.

Change-Id: Ife98b39d5f3a340702151ab27dc8026c8bac12ac
Reviewed-on: http://gerrit.cloudera.org:8080/4574
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/server/server_base.cc
5 files changed, 30 insertions(+), 10 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/4574
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ife98b39d5f3a340702151ab27dc8026c8bac12ac
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] rpc: Add min / max negotiation threads

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: rpc: Add min / max negotiation threads
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4574/1//COMMIT_MSG
Commit Message:

PS1, Line 9: it would be
           : helpful for users to be able to specify the minimum and maximum number
           : of threads in that pool
I can see how this can be useful to reduce flakiness in TSAN-based tests (as you did in disk_reservation-itest), but how is it useful beyond that?

Besides, I thought you were going to address the flakiness by just increasing FLAGS_rpc_negotiation_timeout_ms. Why this approach instead? Neither are particularly invasive, but tweaking an existing flag seems simpler and more predictable than prespawning threads (for the purpose of working around TSAN slow thread creation).


PS1, Line 13: labelled
http://grammarist.com/spelling/label

You move to London and this is what happens?


-- 
To view, visit http://gerrit.cloudera.org:8080/4574
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife98b39d5f3a340702151ab27dc8026c8bac12ac
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] rpc: Add min / max negotiation threads

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4574

to look at the new patch set (#2).

Change subject: rpc: Add min / max negotiation threads
......................................................................

rpc: Add min / max negotiation threads

Since we use a thread pool for connection negotiation, it would be
helpful for users to be able to specify the minimum and maximum number
of threads in that pool. Prior to this patch, there were no gflags to
control these parameters, and the only builder parameter was the maximum
(however it was not labelled as such).

This allows tuning of the per-tserver negotiation pool size, both
minimum and maximum threads to allow in the thread pool.

Change-Id: Ife98b39d5f3a340702151ab27dc8026c8bac12ac
---
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/server/server_base.cc
5 files changed, 30 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/4574/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4574
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife98b39d5f3a340702151ab27dc8026c8bac12ac
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] rpc: Add min / max negotiation threads

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: rpc: Add min / max negotiation threads
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4574/1/src/kudu/rpc/messenger.h
File src/kudu/rpc/messenger.h:

PS1, Line 87: int
> Semantically, is it possible to get negative number here (e.g., like for un
typically we try to use signed ints and assertions for "it shouldn't be negative" rather than uints, since uints have the surprising behavior of wrapping to super-large positive numbers when you least expect it.

https://google.github.io/styleguide/cppguide.html#Integer_Types


-- 
To view, visit http://gerrit.cloudera.org:8080/4574
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife98b39d5f3a340702151ab27dc8026c8bac12ac
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] rpc: Add min / max negotiation threads

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: rpc: Add min / max negotiation threads
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4574/1/src/kudu/rpc/messenger.h
File src/kudu/rpc/messenger.h:

PS1, Line 87: int
Semantically, is it possible to get negative number here (e.g., like for unlimited number of ...)?  If not, consider using unsigned types.

The same for set_max_negotiation_threads() and corresponding class members.


-- 
To view, visit http://gerrit.cloudera.org:8080/4574
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife98b39d5f3a340702151ab27dc8026c8bac12ac
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] rpc: Add min / max negotiation threads

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: rpc: Add min / max negotiation threads
......................................................................


Patch Set 3: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/4574
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife98b39d5f3a340702151ab27dc8026c8bac12ac
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] rpc: Add min / max negotiation threads

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: rpc: Add min / max negotiation threads
......................................................................


Patch Set 1:

(1 comment)

Doing some tests on a very large cluster recently I ran into the fact that this wasn't configurable, so I think making the max configurable is a good idea.

http://gerrit.cloudera.org:8080/#/c/4574/1/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:

PS1, Line 65: 4,
after some experimentation on clusters, I think we should bump this up substantially to 50+


-- 
To view, visit http://gerrit.cloudera.org:8080/4574
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife98b39d5f3a340702151ab27dc8026c8bac12ac
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] rpc: Add min / max negotiation threads

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: rpc: Add min / max negotiation threads
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4574/1//COMMIT_MSG
Commit Message:

PS1, Line 9: it would be
           : helpful for users to be able to specify the minimum and maximum number
           : of threads in that pool
> I can see how this can be useful to reduce flakiness in TSAN-based tests (a
The flaky tests were solved in a different manner, but this is still useful for tuning in general so let's bring it in.


PS1, Line 13: labelled
> http://grammarist.com/spelling/label
Usually, American English makes more sense...


-- 
To view, visit http://gerrit.cloudera.org:8080/4574
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife98b39d5f3a340702151ab27dc8026c8bac12ac
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes