zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From eribeiro <...@git.apache.org>
Subject [GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Date Tue, 08 May 2018 11:22:37 GMT
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?


---

Mime
View raw message