Skip to content

Fix for #572. Refresh connection IP address info with (possibly) new DNS info when creating a new connection.#573

Merged
alexmoore merged 10 commits intodevelopfrom
fixes/am/572
Mar 2, 2016
Merged

Fix for #572. Refresh connection IP address info with (possibly) new DNS info when creating a new connection.#573
alexmoore merged 10 commits intodevelopfrom
fixes/am/572

Conversation

@alexmoore
Copy link
Contributor

1. Problem

If you used the Java Client in a cloud environment where IP addresses for DNS entries could change often, the Java client would not expire the old DNS lookup to get the newest IP address.

2. Solution

Force a refresh any time a connection is made outside of the cluster/node start() method. (Lookup is done once at start).
https://github.com/basho/riak-java-client/pull/573/files#diff-28c20364743c5fc78c7c41c7c8b89e88R293

Also added a log warning if they have their DNS cache setting set to "infinity", which would also cause this issue: https://github.com/basho/riak-java-client/pull/573/files#diff-28c20364743c5fc78c7c41c7c8b89e88R925

3. Test

  1. Setup a "good" mock Netty Bootstrap / ChannelFuture / Channel with a RiakNode
  2. Start RiakNode, should do address lookup Import trifork's PBC Client, Map/Reduce filter support #1.
  3. Fetch/return 1 good connection.
  4. Force the mock to return false for channel.isOpen() when we get the next connection, which should force a second lookup.
  5. Get next connection.
  6. Verify that there were two address lookups.

https://github.com/basho/riak-java-client/pull/573/files#diff-4361a4b26a0905c4782f996ce653ccaaR416

@alexmoore
Copy link
Contributor Author

Assigning @lukebakken and @jenagraham for review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dat space char

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, got rid of all the crazy tabs at some point.

@lukebakken
Copy link
Contributor

@alexmoore 🍙 is in your court 🍥

try
{
channel = doGetConnection();
channel = doGetConnection(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems weird that we get a connection during shutdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do we get a connection on shutdown?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that what is happening here? shutdown() method calls doGetConnection ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I can see, the only places we call doGetConnection/1 are in start(), the general getConnection/1 method, and the health checker.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh stupid GH code folding. Even when I expanded the diff it still wasn't clear. Sorry!

@lukebakken
Copy link
Contributor

@alexmoore - any chance of merging in that other fix that will fix the build?

@alexmoore
Copy link
Contributor Author

This still needs fixes from #594.

@christophermancini
Copy link

👍 Tests passed and coverage looks good.

@christophermancini
Copy link

👍 f31e557

@alexmoore
Copy link
Contributor Author

@borshop +1 f31e557

alexmoore added a commit that referenced this pull request Mar 2, 2016
Fix for #572. Refresh connection IP address info with (possibly) new DNS info when creating a new connection.
@alexmoore alexmoore merged commit 49491f5 into develop Mar 2, 2016
@alexmoore alexmoore deleted the fixes/am/572 branch March 2, 2016 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants