Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache InetSocketAddress if hostname is IPAddress #1789

Merged
merged 3 commits into from Nov 8, 2018

Conversation

@reddycharan
Copy link
Contributor

reddycharan commented Nov 6, 2018

Descriptions of the changes in this PR:

  • in BookieSocketAddress if IPAddress is hostname then it is okay
    to cache InetSocketAddress, since the canonicalhostname of the node
    dont change.
- in BookieSocketAddress if IPAddress is hostname then it is okay
to cache InetSocketAddress, since the canonicalhostname of the node
dont change.
@reddycharan reddycharan requested a review from merlimat Nov 6, 2018
Copy link
Contributor

eolivelli left a comment

Good.

What about adding some test case?
I am interested in having tests with ipv6 addresses for instance

@merlimat

This comment has been minimized.

Copy link
Contributor

merlimat commented Nov 6, 2018

@reddycharan One other option could be to use InetSocketAddress.createUnresolved(), though I haven't tested if the cached gets updated when the name is finally resolved. If it doesn't, it might be a good solution too.

@reddycharan

This comment has been minimized.

Copy link
Contributor Author

reddycharan commented Nov 6, 2018

@merlimat we cannt use InetSocketAddress.createUnresolved() here, because InetSocketAddress.getAddress would return null if it is unresolved https://docs.oracle.com/javase/7/docs/api/java/net/InetSocketAddress.html#getAddress()

in many places in our code we would use address of InetSocketAddress -

if (addr.getSocketAddress().getAddress().isLoopbackAddress()

https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/helpers/CommandHelpers.java#L44

@@ -38,11 +39,19 @@
// Member fields that make up this class.
private final String hostname;
private final int port;
private final boolean isHostnameIPAddress;
private final InetSocketAddress socketAddress;

This comment has been minimized.

Copy link
@dlg99

dlg99 Nov 6, 2018

Contributor

I'd do Optional<InetSocketAddress>
assign Optional.empty() if it should not be cached
and use
socketAddress.orElseGet(() -> new InetSocketAddress(hostname, port)) to get address as needed.

This comment has been minimized.

Copy link
@reddycharan

reddycharan Nov 7, 2018

Author Contributor

fixed it

@reddycharan

This comment has been minimized.

Copy link
Contributor Author

reddycharan commented Nov 6, 2018

@eolivelli regarding testing I'm in the same boat as Matteo here, 3d8bad4

for ipv4/ipv6 i can confirm that InetAddresses.isInetAddress handles both ipv4/ipv6 - https://github.com/google/guava/blob/master/guava/src/com/google/common/net/InetAddresses.java#L186

@reddycharan

This comment has been minimized.

Copy link
Contributor Author

reddycharan commented Nov 7, 2018

@eolivelli actually i just noticed that our current code wouldn't work with IPV6 address format, https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/BookieSocketAddress.java#L50 here (and other places) we are assuming that COLON is the separator between bookie hostname/ipaddress and port in the bookieid.

but in the ipv6 format ':' is used instead of '.' in its ipv6 address string representation - for eg - 2001:db8:85a3:0:0:8a2e:370:7334

@sijie

This comment has been minimized.

Copy link
Member

sijie commented Nov 7, 2018

fyi it is a known issue that ipv6 is not supported #1513

- addressing review comments
@reddycharan

This comment has been minimized.

Copy link
Contributor Author

reddycharan commented Nov 7, 2018

@eolivelli added basic testcases validating cache functionality.

@@ -38,11 +40,26 @@
// Member fields that make up this class.
private final String hostname;
private final int port;
private final boolean isHostnameIPAddress;

This comment has been minimized.

Copy link
@merlimat

merlimat Nov 7, 2018

Contributor

With “optional” the Boolean shouldn’t be needed anymore

This comment has been minimized.

Copy link
@reddycharan

reddycharan Nov 7, 2018

Author Contributor

removed boolean variable

Copy link
Contributor

eolivelli left a comment

I can't see the new test. Maybe you did not push

- review comments
@reddycharan

This comment has been minimized.

Copy link
Contributor Author

reddycharan commented Nov 7, 2018

@eolivelli my bad. fixed it.

Copy link
Contributor

merlimat left a comment

👍

@sijie
sijie approved these changes Nov 7, 2018
@sijie

This comment has been minimized.

Copy link
Member

sijie commented Nov 7, 2018

run integration tests

1 similar comment
@sijie

This comment has been minimized.

Copy link
Member

sijie commented Nov 7, 2018

run integration tests

* instance, if bookies are advertising hostnames and the IP change, the
* BK client will keep forever to try to connect to the old IP.
*/
return socketAddress.orElseGet(() -> {

This comment has been minimized.

Copy link
@dlg99

dlg99 Nov 7, 2018

Contributor

nit:

        return socketAddress.orElseGet(() -> new InetSocketAddress(hostname, port));

This comment has been minimized.

Copy link
@reddycharan

reddycharan Nov 7, 2018

Author Contributor

multiline was suggestion from code formatter.

@dlg99
dlg99 approved these changes Nov 7, 2018
@sijie sijie added this to the 4.9.0 milestone Nov 8, 2018
@sijie sijie merged commit 102475a into apache:master Nov 8, 2018
10 checks passed
10 checks passed
Jenkins: All Other Tests SUCCESS
Details
Jenkins: Bookie Tests SUCCESS
Details
Jenkins: Build (Java 8) SUCCESS
Details
Jenkins: Build (Java 9) SUCCESS
Details
Jenkins: Client Tests SUCCESS
Details
Jenkins: Integration Tests SUCCESS
Details
Jenkins: PR Validation SUCCESS
Details
Jenkins: Replication Tests SUCCESS
Details
Jenkins: TLS Tests SUCCESS
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sijie sijie added the release/4.8.2 label Nov 8, 2018
sijie added a commit that referenced this pull request Nov 8, 2018
Descriptions of the changes in this PR:

- in BookieSocketAddress if IPAddress is hostname then it is okay
to cache InetSocketAddress, since the canonicalhostname of the node
dont change.

Reviewers: Sijie Guo <sijie@apache.org>, Enrico Olivelli <eolivelli@gmail.com>, Matteo Merli <mmerli@apache.org>, Andrey Yegorov <None>

This closes #1789 from reddycharan/bookieaddressonlywhenhostname

(cherry picked from commit 102475a)
Signed-off-by: Sijie Guo <sijie@apache.org>
@sijie sijie added the release/4.7.3 label Nov 8, 2018
sijie added a commit that referenced this pull request Nov 8, 2018
Descriptions of the changes in this PR:

- in BookieSocketAddress if IPAddress is hostname then it is okay
to cache InetSocketAddress, since the canonicalhostname of the node
dont change.

Reviewers: Sijie Guo <sijie@apache.org>, Enrico Olivelli <eolivelli@gmail.com>, Matteo Merli <mmerli@apache.org>, Andrey Yegorov <None>

This closes #1789 from reddycharan/bookieaddressonlywhenhostname

(cherry picked from commit 102475a)
Signed-off-by: Sijie Guo <sijie@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.