You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2018/04/19 12:26:54 UTC
[kudu-CR] KUDU-2368 Configure num reactors in client
Attila Bukor has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10115
Change subject: KUDU-2368 Configure num_reactors in client
......................................................................
KUDU-2368 Configure num_reactors in client
The number of reactors was not configurable from the KuduClient, it
always used the default number of 4.
This patch adds ability to configure the number of reactors in
KuduClientBuilder.
Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client_builder-internal.h
3 files changed, 16 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/10115/1
--
To view, visit http://gerrit.cloudera.org:8080/10115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
Gerrit-Change-Number: 10115
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
[kudu-CR] KUDU-2368 Configure num reactors in client
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10115
to look at the new patch set (#2).
Change subject: KUDU-2368 Configure num_reactors in client
......................................................................
KUDU-2368 Configure num_reactors in client
The number of reactors was not configurable from the KuduClient, it
always used the default number of 4.
This patch adds ability to configure the number of reactors in
KuduClientBuilder.
Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client_builder-internal.h
3 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/10115/2
--
To view, visit http://gerrit.cloudera.org:8080/10115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
Gerrit-Change-Number: 10115
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2368 Configure num reactors in client
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/10115 )
Change subject: KUDU-2368 Configure num_reactors in client
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10115/2/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/10115/2/src/kudu/client/client.h@252
PS2, Line 252: /// the underlying messenger is created with the default number of reactor
> typo
Done
--
To view, visit http://gerrit.cloudera.org:8080/10115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
Gerrit-Change-Number: 10115
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 23 Apr 2018 20:25:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2368 Configure num reactors in client
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/10115 )
Change subject: KUDU-2368 Configure num_reactors in client
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client.h@249
PS1, Line 249: @brief Set the number of r
> Since this is a public API we should probably explain further what this mea
I'm not quite sure about how this should be set, but I expanded the text on what I know. Is this enough or should anything else be added?
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client.h@251
PS1, Line 251: The reactor threads are used fo
> Yea I think it's better to be vague here and say that if not specified, the
Done
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client_builder-internal.h
File src/kudu/client/client_builder-internal.h:
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client_builder-internal.h@42
PS1, Line 42: :string authn
> yea, seems this should be optional<> and then only call the setter on the m
Done
--
To view, visit http://gerrit.cloudera.org:8080/10115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
Gerrit-Change-Number: 10115
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 23 Apr 2018 16:15:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2368 Configure num reactors in client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10115 )
Change subject: KUDU-2368 Configure num_reactors in client
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client.h@249
PS1, Line 249: Set the number of reactors
> Set the number of reactors for the RPC messenger.
Since this is a public API we should probably explain further what this means, how someone might want to set it, etc.
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client.h@251
PS1, Line 251: If not provided, defaults to 4.
> Another approach might be just using the messenger's default number of reac
Yea I think it's better to be vague here and say that if not specified, the implementation will choose a value. Currently the choosing is hard-coded 4 but in the future we may choose the number of CPUs or somesuch
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client_builder-internal.h
File src/kudu/client/client_builder-internal.h:
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client_builder-internal.h@42
PS1, Line 42: num_reactors_
> Where is the default value for this num_reactors_ is assigned? Maybe, it s
yea, seems this should be optional<> and then only call the setter on the messenger if necessary
--
To view, visit http://gerrit.cloudera.org:8080/10115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
Gerrit-Change-Number: 10115
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Apr 2018 01:10:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2368 Configure num reactors in client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10115 )
Change subject: KUDU-2368 Configure num_reactors in client
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client.h@249
PS1, Line 249: Set the number of reactors
Set the number of reactors for the RPC messenger.
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client_builder-internal.h
File src/kudu/client/client_builder-internal.h:
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client_builder-internal.h@42
PS1, Line 42: num_reactors_
Where is the default value for this num_reactors_ is assigned? Maybe, it should be done in the KuduClientBuilder::Data's constructor?
--
To view, visit http://gerrit.cloudera.org:8080/10115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
Gerrit-Change-Number: 10115
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Apr 2018 19:39:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2368 Configure num reactors in client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10115 )
Change subject: KUDU-2368 Configure num_reactors in client
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
Gerrit-Change-Number: 10115
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 23 Apr 2018 20:54:20 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2368 Configure num reactors in client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10115 )
Change subject: KUDU-2368 Configure num_reactors in client
......................................................................
Patch Set 2:
(1 comment)
one nit, otherwise looks good
http://gerrit.cloudera.org:8080/#/c/10115/2/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/10115/2/src/kudu/client/client.h@252
PS2, Line 252: /// the underlying messenger is created with the default number of rector
typo
--
To view, visit http://gerrit.cloudera.org:8080/10115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
Gerrit-Change-Number: 10115
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 23 Apr 2018 19:25:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2368 Configure num reactors in client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10115 )
Change subject: KUDU-2368 Configure num_reactors in client
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/10115/1/src/kudu/client/client.h@251
PS1, Line 251: If not provided, defaults to 4.
Another approach might be just using the messenger's default number of reactors if KuduClientBuilder::num_reactors() is not called, and just say the underlying messenger is created with the default number of reactor threads.
--
To view, visit http://gerrit.cloudera.org:8080/10115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
Gerrit-Change-Number: 10115
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Apr 2018 19:42:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2368 Configure num reactors in client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10115 )
Change subject: KUDU-2368 Configure num_reactors in client
......................................................................
KUDU-2368 Configure num_reactors in client
The number of reactors was not configurable from the KuduClient, it
always used the default number of 4.
This patch adds ability to configure the number of reactors in
KuduClientBuilder.
Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
Reviewed-on: http://gerrit.cloudera.org:8080/10115
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client_builder-internal.h
3 files changed, 22 insertions(+), 0 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/10115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
Gerrit-Change-Number: 10115
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2368 Configure num reactors in client
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10115
to look at the new patch set (#3).
Change subject: KUDU-2368 Configure num_reactors in client
......................................................................
KUDU-2368 Configure num_reactors in client
The number of reactors was not configurable from the KuduClient, it
always used the default number of 4.
This patch adds ability to configure the number of reactors in
KuduClientBuilder.
Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client_builder-internal.h
3 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/10115/3
--
To view, visit http://gerrit.cloudera.org:8080/10115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8320d7d664b8006111e7316643f58b4e32c5081f
Gerrit-Change-Number: 10115
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>