Don't cache Bookie hostname DNS resolution forever

[merlimat]

Motivation

BookieSocketAddress is resolving the bookie DNS name in its constructor and then using the already resolved InetSocketAddress instance.

If the IP of a bookie changes, the BK client will continue to use the old IP address.

Changes

Construct a new InetSocketAddress each time getSocketAddress() gets called (eg: each time we attempt to make a new connection) so that we're making sure to get the right IP.

I cannot think of a good way to add unit test for this at this point, suggestions are welcome.

I think this should be included in a patch release as well 4.7.3 or 4.8.1

[eolivelli]

Okay.

+1 for not adding tests, it will very tricky

But we should add a comment, in the future people could think it will be a good idea to cache the value and undo this patch

[merlimat]

But we should add a comment, in the future people could think it will be a good idea to cache the value and undo this patch

Good point, I'll add comment. In future we should try to move to use async DNS from Netty instead of relying on the blocking DNS resolution from JDK + OS. We've been using that in Pulsar for 1 year now and it has been working very well.

[ivankelly]

I think unit testing would be hard. Should be possible with an integration test, but I'm not sure if it's worth it.

[merlimat]

@eolivelli Added comment

[dlg99]

LGTM but you also need to consider effects of
-Dnetworkaddress.cache.ttl jvm option.

networkaddress.cache.ttl
Specified in java.security to indicate the caching policy for successful name lookups from the name service.. 
The value is specified as integer to indicate the number of seconds to cache the successful lookup.

A value of -1 indicates "cache forever". 

The default behavior is to cache forever when a security manager is installed, 
and to cache for an implementation specific period of time, when a security manager is not installed.

[merlimat]

@dlg99 -Dnetworkaddress.cache.ttl cannot be passed as system property from CLI but rather included in java.security property file.

In any case, the current behavior is caching the DNS resolution forever in the BK client process. That comes before reaching the JVM DNS cache or the OS DNS cache. By creating new instances of InetSocketAddress we will rely on the JVM cache, that depends on the networkaddress.cache.ttl setting.

(BTW: I think the JVM cache is a bad thing on its own, but that's a different story... :) )

[merlimat]

run integration tests

[sijie]

IGNORE IT CI

[eolivelli]

+1
Looks good. Thanks

[sijie]

merged to master, branch-4.8 and branch-4.7