You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2018/07/17 20:34:55 UTC
[kudu-CR] [java] Add extra info about using multiple clients
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10962
Change subject: [java] Add extra info about using multiple clients
......................................................................
[java] Add extra info about using multiple clients
The AsyncKuduClient and KuduClient are meant to map 1-1 with Kudu
clusters used by an application. In particular, an application shouldn't
make more than one client per cluster. This patch adds a note to this
effect to the AsyncKuduClient javadoc.
Additionally, some use cases require using multiple clients in multiple
applications running on the same machine. For example, to provide very
strict classpath isolation, or to run applications on different JVM
versions, or even for extra security by isolating different data flows
in different processes. In this case, I've seen client instances put
pressure on thread limits on machines with a lot of cores because the
AsyncKuduClient starts at least (2 * #cpu) threads for a netty threadpool.
This patch also adds advice to adjust the netty worker count to help
users with this case.
Change-Id: I34242862ee6d806b74d7717399e5fd0260dece89
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
1 file changed, 10 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/10962/1
--
To view, visit http://gerrit.cloudera.org:8080/10962
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I34242862ee6d806b74d7717399e5fd0260dece89
Gerrit-Change-Number: 10962
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
[kudu-CR] [java] Add extra info about using multiple clients
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10962 )
Change subject: [java] Add extra info about using multiple clients
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@85
PS1, Line 85: that
not from this changelist, but just a small nit: that --> when (take it with a grain of salt)
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@91
PS1, Line 91: See the options in {@link AsyncKuduClientBuilder}
Maybe, it's worth pointing to KuduClientBuilder as well?
--
To view, visit http://gerrit.cloudera.org:8080/10962
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34242862ee6d806b74d7717399e5fd0260dece89
Gerrit-Change-Number: 10962
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 19 Jul 2018 00:04:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add extra info about using multiple clients
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10962 )
Change subject: [java] Add extra info about using multiple clients
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@86
PS1, Line 86:
: *
: * In rare cases where a single app
> Are you suggesting making those two points in a bulleted list? I really don
Nope, separating out into two sections as you did was pretty much what I had in mind.
--
To view, visit http://gerrit.cloudera.org:8080/10962
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34242862ee6d806b74d7717399e5fd0260dece89
Gerrit-Change-Number: 10962
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 17:30:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add extra info about using multiple clients
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10962 )
Change subject: [java] Add extra info about using multiple clients
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@83
PS1, Line 83: .
> nit: not your fault, but comma
Done
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@85
PS1, Line 85: that
> not from this changelist, but just a small nit: that --> when (take it with
Done
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@86
PS1, Line 86: It's generally
: * not appropriate to use multiple client instances connected to the same
: * cluster in a single application.
> nit: maybe move this before "Separate client instances should be..." and a
Are you suggesting making those two points in a bulleted list? I really don't like that because the points aren't parallel. I reworked the section to be briefer.
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@91
PS1, Line 91: See the options in {@link AsyncKuduClientBuilder}
> Maybe, it's worth pointing to KuduClientBuilder as well?
The sync client and builder refer to their async counterparts for docs.
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2267
PS1, Line 2267: appropriately
> nit: It wasn't super obvious what "appropriately" meant here until I read t
Done
--
To view, visit http://gerrit.cloudera.org:8080/10962
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34242862ee6d806b74d7717399e5fd0260dece89
Gerrit-Change-Number: 10962
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 16:55:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add extra info about using multiple clients
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10962 )
Change subject: [java] Add extra info about using multiple clients
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@83
PS1, Line 83: .
nit: not your fault, but comma
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@86
PS1, Line 86: It's generally
: * not appropriate to use multiple client instances connected to the same
: * cluster in a single application.
nit: maybe move this before "Separate client instances should be..." and add a newline to separate the points we're making:
* don't use multiple clients to point to the same cluster
* if you really want multiple clients, adjust your resources
http://gerrit.cloudera.org:8080/#/c/10962/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2267
PS1, Line 2267: appropriately
nit: It wasn't super obvious what "appropriately" meant here until I read the commit message, so maybe specify "to abide thread resource limits" or something?
--
To view, visit http://gerrit.cloudera.org:8080/10962
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34242862ee6d806b74d7717399e5fd0260dece89
Gerrit-Change-Number: 10962
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 18 Jul 2018 17:15:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] Add extra info about using multiple clients
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10962
to look at the new patch set (#2).
Change subject: [java] Add extra info about using multiple clients
......................................................................
[java] Add extra info about using multiple clients
The AsyncKuduClient and KuduClient are meant to map 1-1 with Kudu
clusters used by an application. In particular, an application shouldn't
make more than one client per cluster. This patch adds a note to this
effect to the AsyncKuduClient javadoc.
Additionally, some use cases require using multiple clients in multiple
applications running on the same machine. For example, to provide very
strict classpath isolation, or to run applications on different JVM
versions, or even for extra security by isolating different data flows
in different processes. In this case, I've seen client instances put
pressure on thread limits on machines with a lot of cores because the
AsyncKuduClient starts at least (2 * #cpu) threads for a netty threadpool.
This patch also adds advice to adjust the netty worker count to help
users with this case.
Change-Id: I34242862ee6d806b74d7717399e5fd0260dece89
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
1 file changed, 14 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/10962/2
--
To view, visit http://gerrit.cloudera.org:8080/10962
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I34242862ee6d806b74d7717399e5fd0260dece89
Gerrit-Change-Number: 10962
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java] Add extra info about using multiple clients
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10962 )
Change subject: [java] Add extra info about using multiple clients
......................................................................
[java] Add extra info about using multiple clients
The AsyncKuduClient and KuduClient are meant to map 1-1 with Kudu
clusters used by an application. In particular, an application shouldn't
make more than one client per cluster. This patch adds a note to this
effect to the AsyncKuduClient javadoc.
Additionally, some use cases require using multiple clients in multiple
applications running on the same machine. For example, to provide very
strict classpath isolation, or to run applications on different JVM
versions, or even for extra security by isolating different data flows
in different processes. In this case, I've seen client instances put
pressure on thread limits on machines with a lot of cores because the
AsyncKuduClient starts at least (2 * #cpu) threads for a netty threadpool.
This patch also adds advice to adjust the netty worker count to help
users with this case.
Change-Id: I34242862ee6d806b74d7717399e5fd0260dece89
Reviewed-on: http://gerrit.cloudera.org:8080/10962
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
1 file changed, 14 insertions(+), 4 deletions(-)
Approvals:
Andrew Wong: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/10962
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I34242862ee6d806b74d7717399e5fd0260dece89
Gerrit-Change-Number: 10962
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>