You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/03/14 23:29:34 UTC
[kudu-CR] java: key ConnectionCache by address, improve stringification
Hello Will Berkeley, Grant Henke,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/9643
to review the following change.
Change subject: java: key ConnectionCache by address, improve stringification
......................................................................
java: key ConnectionCache by address, improve stringification
This changes the ConnectionCache in the Java client to be keyed by
InetSocketAddress instead of server UUID. Although we currently assume
that an address and UUID have a 1:1 correspondence, the logical purpose
of the connection cache is to cache a connection to a specific server
endpoint, and if a server were to re-register at a new IP, we'd really
need to reconnect.
Along the way this also adds and improves toString() methods for a
number of core structures in the Java client. These new stringifications
helped me debug a recent issue in the client where it wasn't clear that
we were connecting to the wrong host.
Change-Id: I431b5b6b8af6b81e6ab494d7f84fa2260bb0f941
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestTableLocationsCache.java
10 files changed, 167 insertions(+), 87 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9643/1
--
To view, visit http://gerrit.cloudera.org:8080/9643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I431b5b6b8af6b81e6ab494d7f84fa2260bb0f941
Gerrit-Change-Number: 9643
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] java: key ConnectionCache by address, improve stringification
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9643 )
Change subject: java: key ConnectionCache by address, improve stringification
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9643/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java:
http://gerrit.cloudera.org:8080/#/c/9643/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java@74
PS1, Line 74: private final HashMultimap<InetSocketAddress, Connection> connectionsByIp =
Nit: connectionsByAddress?
http://gerrit.cloudera.org:8080/#/c/9643/1/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java:
http://gerrit.cloudera.org:8080/#/c/9643/1/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@189
PS1, Line 189: // TODO(todd) this doesn't return a random server, but rather returns
Open a newbie jira to track/fix it?
http://gerrit.cloudera.org:8080/#/c/9643/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java:
http://gerrit.cloudera.org:8080/#/c/9643/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java@a59
PS1, Line 59:
Was this just unnecessary?
--
To view, visit http://gerrit.cloudera.org:8080/9643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I431b5b6b8af6b81e6ab494d7f84fa2260bb0f941
Gerrit-Change-Number: 9643
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 00:00:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: key ConnectionCache by address, improve stringification
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9643 )
Change subject: java: key ConnectionCache by address, improve stringification
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9643/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java:
http://gerrit.cloudera.org:8080/#/c/9643/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java@74
PS1, Line 74: private final HashMultimap<InetSocketAddress, Connection> connectionsByIp =
> Nit: connectionsByAddress?
went with connsByAddress since connectionsByAddress made some longs pretty long
http://gerrit.cloudera.org:8080/#/c/9643/1/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java:
http://gerrit.cloudera.org:8080/#/c/9643/1/java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java@189
PS1, Line 189: // TODO(todd) this doesn't return a random server, but rather returns
> Open a newbie jira to track/fix it?
Done
--
To view, visit http://gerrit.cloudera.org:8080/9643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I431b5b6b8af6b81e6ab494d7f84fa2260bb0f941
Gerrit-Change-Number: 9643
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 00:44:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: key ConnectionCache by address, improve stringification
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9643 )
Change subject: java: key ConnectionCache by address, improve stringification
......................................................................
Patch Set 1:
> Patch Set 1:
>
> Is AsyncKuduClient.getFakeMasterUuid() still needed or useful with this patch?
Well, our Connection class still uses ServerInfo instead of just an IP address, and ServerInfo requires a UUID. SO, while getFakeMasterUuid() being consistent across usages isn't required for correctness anymore, it's still nice to ensure we have a reasonable log instead of an empty string when we try to log the UUID
--
To view, visit http://gerrit.cloudera.org:8080/9643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I431b5b6b8af6b81e6ab494d7f84fa2260bb0f941
Gerrit-Change-Number: 9643
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 00:19:28 +0000
Gerrit-HasComments: No
[kudu-CR] java: key ConnectionCache by address, improve stringification
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9643 )
Change subject: java: key ConnectionCache by address, improve stringification
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9643/2/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java:
http://gerrit.cloudera.org:8080/#/c/9643/2/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@52
PS2, Line 52: Preconditions.checkArgument(hostPort.getPort() > 0);
Add Preconditions.checkNotNull(hostPort); ?
--
To view, visit http://gerrit.cloudera.org:8080/9643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I431b5b6b8af6b81e6ab494d7f84fa2260bb0f941
Gerrit-Change-Number: 9643
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 04:11:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: key ConnectionCache by address, improve stringification
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9643 )
Change subject: java: key ConnectionCache by address, improve stringification
......................................................................
java: key ConnectionCache by address, improve stringification
This changes the ConnectionCache in the Java client to be keyed by
InetSocketAddress instead of server UUID. Although we currently assume
that an address and UUID have a 1:1 correspondence, the logical purpose
of the connection cache is to cache a connection to a specific server
endpoint, and if a server were to re-register at a new IP, we'd really
need to reconnect.
Along the way this also adds and improves toString() methods for a
number of core structures in the Java client. These new stringifications
helped me debug a recent issue in the client where it wasn't clear that
we were connecting to the wrong host.
Change-Id: I431b5b6b8af6b81e6ab494d7f84fa2260bb0f941
Reviewed-on: http://gerrit.cloudera.org:8080/9643
Tested-by: Kudu Jenkins
Reviewed-by: Grant Henke <gr...@gmail.com>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestTableLocationsCache.java
10 files changed, 167 insertions(+), 87 deletions(-)
Approvals:
Kudu Jenkins: Verified
Grant Henke: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/9643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I431b5b6b8af6b81e6ab494d7f84fa2260bb0f941
Gerrit-Change-Number: 9643
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] java: key ConnectionCache by address, improve stringification
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9643 )
Change subject: java: key ConnectionCache by address, improve stringification
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9643/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java:
http://gerrit.cloudera.org:8080/#/c/9643/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java@a59
PS1, Line 59:
> Was this just unnecessary?
it might have been necessarily previously to ensure that it made separate new proxies because we didn't properly share proxies between the 'ConnectToCluster' and the later usage? Not sure, but figured I'd just clean up the redundant unused calls. here.
--
To view, visit http://gerrit.cloudera.org:8080/9643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I431b5b6b8af6b81e6ab494d7f84fa2260bb0f941
Gerrit-Change-Number: 9643
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 00:18:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] java: key ConnectionCache by address, improve stringification
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9643 )
Change subject: java: key ConnectionCache by address, improve stringification
......................................................................
Patch Set 1:
Is AsyncKuduClient.getFakeMasterUuid() still needed or useful with this patch?
--
To view, visit http://gerrit.cloudera.org:8080/9643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I431b5b6b8af6b81e6ab494d7f84fa2260bb0f941
Gerrit-Change-Number: 9643
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 15 Mar 2018 00:17:13 +0000
Gerrit-HasComments: No
[kudu-CR] java: key ConnectionCache by address, improve stringification
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Kudu Jenkins, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9643
to look at the new patch set (#2).
Change subject: java: key ConnectionCache by address, improve stringification
......................................................................
java: key ConnectionCache by address, improve stringification
This changes the ConnectionCache in the Java client to be keyed by
InetSocketAddress instead of server UUID. Although we currently assume
that an address and UUID have a 1:1 correspondence, the logical purpose
of the connection cache is to cache a connection to a specific server
endpoint, and if a server were to re-register at a new IP, we'd really
need to reconnect.
Along the way this also adds and improves toString() methods for a
number of core structures in the Java client. These new stringifications
helped me debug a recent issue in the client where it wasn't clear that
we were connecting to the wrong host.
Change-Id: I431b5b6b8af6b81e6ab494d7f84fa2260bb0f941
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectionCache.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestRemoteTablet.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestTableLocationsCache.java
10 files changed, 167 insertions(+), 87 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9643/2
--
To view, visit http://gerrit.cloudera.org:8080/9643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I431b5b6b8af6b81e6ab494d7f84fa2260bb0f941
Gerrit-Change-Number: 9643
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>