-
Notifications
You must be signed in to change notification settings - Fork 875
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
JAVA-3131: Add #retrieve method to EndPoint for when caller does not … #1735
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,11 @@ public DefaultEndPoint(InetSocketAddress address) { | |
@NonNull | ||
@Override | ||
public InetSocketAddress resolve() { | ||
return retrieve(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: save yourself the method call overhead here and just return address directly... ? |
||
} | ||
|
||
@Override | ||
public InetSocketAddress retrieve() { | ||
return address; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
package com.datastax.oss.driver.internal.core.metadata; | ||
|
||
import com.datastax.oss.driver.api.core.metadata.EndPoint; | ||
import com.datastax.oss.driver.shaded.guava.common.annotations.VisibleForTesting; | ||
import com.datastax.oss.driver.shaded.guava.common.primitives.UnsignedBytes; | ||
import edu.umd.cs.findbugs.annotations.NonNull; | ||
import java.net.InetAddress; | ||
|
@@ -26,10 +27,14 @@ | |
import java.util.Arrays; | ||
import java.util.Comparator; | ||
import java.util.Objects; | ||
import java.util.concurrent.atomic.AtomicLong; | ||
import java.util.concurrent.ThreadLocalRandom; | ||
import java.util.concurrent.atomic.AtomicInteger; | ||
|
||
public class SniEndPoint implements EndPoint { | ||
private static final AtomicLong OFFSET = new AtomicLong(); | ||
// initialize offset to random position to avoid all clients starting at the same index | ||
@VisibleForTesting | ||
static final AtomicInteger OFFSET = | ||
new AtomicInteger(ThreadLocalRandom.current().nextInt(0, 1024)); | ||
|
||
private final InetSocketAddress proxyAddress; | ||
private final String serverName; | ||
|
@@ -55,7 +60,7 @@ public String getServerName() { | |
@Override | ||
public InetSocketAddress resolve() { | ||
try { | ||
InetAddress[] aRecords = InetAddress.getAllByName(proxyAddress.getHostName()); | ||
InetAddress[] aRecords = resolveARecords(); | ||
if (aRecords.length == 0) { | ||
// Probably never happens, but the JDK docs don't explicitly say so | ||
throw new IllegalArgumentException( | ||
|
@@ -64,14 +69,32 @@ public InetSocketAddress resolve() { | |
// The order of the returned address is unspecified. Sort by IP to make sure we get a true | ||
// round-robin | ||
Arrays.sort(aRecords, IP_COMPARATOR); | ||
int index = (aRecords.length == 1) ? 0 : (int) OFFSET.getAndIncrement() % aRecords.length; | ||
return new InetSocketAddress(aRecords[index], proxyAddress.getPort()); | ||
|
||
// get next offset value, reset OFFSET if wrapped around to negative | ||
int nextOffset = OFFSET.getAndIncrement(); | ||
if (nextOffset < 0) { | ||
// if negative set OFFSET to 1 and nextOffset to 0, else simulate getAndIncrement() | ||
nextOffset = OFFSET.updateAndGet(v -> v < 0 ? 1 : v + 1) - 1; | ||
} | ||
|
||
return new InetSocketAddress(aRecords[nextOffset % aRecords.length], proxyAddress.getPort()); | ||
} catch (UnknownHostException e) { | ||
throw new IllegalArgumentException( | ||
"Could not resolve proxy address " + proxyAddress.getHostName(), e); | ||
} | ||
} | ||
|
||
@VisibleForTesting | ||
InetAddress[] resolveARecords() throws UnknownHostException { | ||
// moving static call to method to allow mocking in tests | ||
return InetAddress.getAllByName(proxyAddress.getHostName()); | ||
} | ||
|
||
@Override | ||
public InetSocketAddress retrieve() { | ||
return proxyAddress; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we want to cache the last value returned by resolve() and return that here instead? |
||
} | ||
|
||
@Override | ||
public boolean equals(Object other) { | ||
if (other == this) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it makes sense to provide a default implementation for the new method. It's unlikely we'll be adding a new endpoint type anytime soon but failing to distinguish between "look up the addresses again" and "give me the addresses you got last time you looked them up" is what got us into this situation in the first place. I kinda feel like having this as a default might lend itself to landing back in exactly that situation.