Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186694289
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -30,76 +31,118 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-
/**
* Most simple HostProvider, resolves only on instantiation.
- *
+ *
*/
@InterfaceAudience.Public
public final class StaticHostProvider implements HostProvider {
+ public interface Resolver {
+ InetAddress[] getAllByName(String name) throws
UnknownHostException;
+ }
+
private static final Logger LOG = LoggerFactory
.getLogger(StaticHostProvider.class);
- private final List<InetSocketAddress> serverAddresses = new
ArrayList<InetSocketAddress>(
- 5);
+ private final List<InetSocketAddress> serverAddresses = new
ArrayList<InetSocketAddress>(5);
private int lastIndex = -1;
private int currentIndex = -1;
+ private Resolver resolver;
+
/**
* Constructs a SimpleHostSet.
- *
+ *
* @param serverAddresses
* possibly unresolved ZooKeeper server addresses
* @throws IllegalArgumentException
* if serverAddresses is empty or resolves to an empty list
*/
public StaticHostProvider(Collection<InetSocketAddress>
serverAddresses) {
- for (InetSocketAddress address : serverAddresses) {
- try {
- InetAddress ia = address.getAddress();
- InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
- address.getHostName());
- for (InetAddress resolvedAddress : resolvedAddresses) {
- // If hostName is null but the address is not, we can
tell that
- // the hostName is an literal IP address. Then we can
set the host string as the hostname
- // safely to avoid reverse DNS lookup.
- // As far as i know, the only way to check if the
hostName is null is use toString().
- // Both the two implementations of InetAddress are
final class, so we can trust the return value of
- // the toString() method.
- if (resolvedAddress.toString().startsWith("/")
- && resolvedAddress.getAddress() != null) {
- this.serverAddresses.add(
- new
InetSocketAddress(InetAddress.getByAddress(
- address.getHostName(),
- resolvedAddress.getAddress()),
- address.getPort()));
- } else {
- this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
- }
- }
- } catch (UnknownHostException e) {
- LOG.error("Unable to connect to server: {}", address, e);
+ this.resolver = new Resolver() {
+ @Override
+ public InetAddress[] getAllByName(String name) throws
UnknownHostException {
+ return InetAddress.getAllByName(name);
}
- }
-
- if (this.serverAddresses.isEmpty()) {
+ };
+ init(serverAddresses);
+ }
+
+ /**
+ * Introduced for testing purposes. getAllByName() is a static method
of InetAddress, therefore cannot be easily mocked.
+ * By abstraction of Resolver interface we can easily inject a mocked
implementation in tests.
+ *
+ * @param serverAddresses
+ * possibly unresolved ZooKeeper server addresses
+ * @param resolver
+ * custom resolver implementation
+ * @throws IllegalArgumentException
+ * if serverAddresses is empty or resolves to an empty list
+ */
+ public StaticHostProvider(Collection<InetSocketAddress>
serverAddresses, Resolver resolver) {
+ this.resolver = resolver;
+ init(serverAddresses);
+ }
+
+ /**
+ * Common init method for all constructors.
+ * Resolve all unresolved server addresses, put them in a list and
shuffle.
+ */
+ private void init(Collection<InetSocketAddress> serverAddresses) {
+ if (serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
+
+ this.serverAddresses.addAll(serverAddresses);
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
--- End diff --
As ZK JVM version is jdk7, there is still a need for this method?
---
