Improve readability a bit

Author: ST-DDT

grpc-client-spring-boot-autoconfigure/src/main/java/net/devh/boot/grpc/client/nameresolver/DiscoveryClientNameResolver.java

@@ -20,12 +20,14 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static java.util.Objects.requireNonNull;
+import static net.devh.boot.grpc.common.util.GrpcUtils.CLOUD_DISCOVERY_METADATA_PORT;
 
 import java.net.InetSocketAddress;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Executor;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Consumer;
 
 import org.springframework.cloud.client.ServiceInstance;
 import org.springframework.cloud.client.discovery.DiscoveryClient;
@@ -40,7 +42,6 @@
 import io.grpc.SynchronizationContext;
 import io.grpc.internal.SharedResourceHolder;
 import lombok.extern.slf4j.Slf4j;
-import net.devh.boot.grpc.common.util.GrpcUtils;
 
 /**
  * The DiscoveryClientNameResolver resolves the service hosts and their associated gRPC port using the channel's name
@@ -59,7 +60,7 @@ public class DiscoveryClientNameResolver extends NameResolver {
     private final String name;
     private final DiscoveryClient client;
     private final SynchronizationContext syncContext;
-    private final Runnable externalCleaner;
+    private final Consumer<DiscoveryClientNameResolver> shutdownHook;
     private final SharedResourceHolder.Resource<Executor> executorResource;
     private final boolean usingExecutorResource;
 
@@ -78,27 +79,46 @@ public class DiscoveryClientNameResolver extends NameResolver {
      * @param client The client used to look up the service addresses.
      * @param args The name resolver args.
      * @param executorResource The executor resource.
-     * @param externalCleaner The optional cleaner used during {@link #shutdown()}
+     * @param shutdownHook The optional cleaner used during {@link #shutdown()}
      */
     public DiscoveryClientNameResolver(final String name, final DiscoveryClient client, final Args args,
-            final SharedResourceHolder.Resource<Executor> executorResource, final Runnable externalCleaner) {
+            final SharedResourceHolder.Resource<Executor> executorResource,
+            final Consumer<DiscoveryClientNameResolver> shutdownHook) {
         this.name = name;
         this.client = client;
         this.syncContext = requireNonNull(args.getSynchronizationContext(), "syncContext");
-        this.externalCleaner = externalCleaner;
+        this.shutdownHook = shutdownHook;
         this.executor = args.getOffloadExecutor();
         this.usingExecutorResource = this.executor == null;
         this.executorResource = executorResource;
     }
 
+    /**
+     * Gets the name of the service to get the instances of.
+     *
+     * @return The name associated with this resolver.
+     */
+    protected final String getName() {
+        return this.name;
+    }
+
+    /**
+     * Checks whether this resolver is active. E.g. {@code #start} has been called, but not {@code #shutdown()}.
+     *
+     * @return True, if there is a listener attached. False, otherwise.
+     */
+    protected final boolean isActive() {
+        return this.listener != null;
+    }
+
     @Override
     public final String getServiceAuthority() {
         return this.name;
     }
 
     @Override
     public void start(final Listener2 listener) {
-        checkState(this.listener == null, "already started");
+        checkState(!isActive(), "already started");
         if (this.usingExecutorResource) {
             this.executor = SharedResourceHolder.get(this.executorResource);
         }
@@ -108,7 +128,7 @@ public void start(final Listener2 listener) {
 
     @Override
     public void refresh() {
-        checkState(this.listener != null, "not started");
+        checkState(isActive(), "not started");
         resolve();
     }
 
@@ -120,19 +140,28 @@ public void refresh() {
      */
     public void refreshFromExternal() {
         this.syncContext.execute(() -> {
-            if (this.listener != null) {
+            if (isActive()) {
                 resolve();
             }
         });
     }
 
+    /**
+     * Discovers matching service instances.
+     *
+     * @return A list of service instances to use.
+     */
+    private List<ServiceInstance> discoverServices() {
+        return this.client.getInstances(this.name);
+    }
+
     private void resolve() {
         log.debug("Scheduled resolve for {}", this.name);
         if (this.resolving) {
             return;
         }
         this.resolving = true;
-        this.executor.execute(new Resolve(this.listener, this.instanceList));
+        this.executor.execute(new Resolve(this.listener));
     }
 
     @Override
@@ -142,8 +171,8 @@ public void shutdown() {
             this.executor = SharedResourceHolder.release(this.executorResource, this.executor);
         }
         this.instanceList = Lists.newArrayList();
-        if (this.externalCleaner != null) {
-            this.externalCleaner.run();
+        if (this.shutdownHook != null) {
+            this.shutdownHook.accept(this);
         }
     }
 
@@ -158,17 +187,14 @@ public String toString() {
     private final class Resolve implements Runnable {
 
         private final Listener2 savedListener;
-        private final List<ServiceInstance> savedInstanceList;
 
         /**
          * Creates a new Resolve that stores a snapshot of the relevant states of the resolver.
          *
          * @param listener The listener to send the results to.
-         * @param instanceList The current server instance list.
          */
-        Resolve(final Listener2 listener, final List<ServiceInstance> instanceList) {
+        Resolve(final Listener2 listener) {
             this.savedListener = requireNonNull(listener, "listener");
-            this.savedInstanceList = requireNonNull(instanceList, "instanceList");
         }
 
         @Override
@@ -178,13 +204,13 @@ public void run() {
                 resultContainer.set(resolveInternal());
             } catch (final Exception e) {
                 this.savedListener.onError(Status.UNAVAILABLE.withCause(e)
-                        .withDescription("Failed to update server list for " + DiscoveryClientNameResolver.this.name));
+                        .withDescription("Failed to update server list for " + getName()));
                 resultContainer.set(Lists.newArrayList());
             } finally {
                 DiscoveryClientNameResolver.this.syncContext.execute(() -> {
                     DiscoveryClientNameResolver.this.resolving = false;
                     final List<ServiceInstance> result = resultContainer.get();
-                    if (result != KEEP_PREVIOUS && DiscoveryClientNameResolver.this.listener != null) {
+                    if (result != KEEP_PREVIOUS && isActive()) {
                         DiscoveryClientNameResolver.this.instanceList = result;
                     }
                 });
@@ -198,37 +224,36 @@ public void run() {
          *         should be used.
          */
         private List<ServiceInstance> resolveInternal() {
-            final String name = DiscoveryClientNameResolver.this.name;
-            final List<ServiceInstance> newInstanceList =
-                    DiscoveryClientNameResolver.this.client.getInstances(name);
-            log.debug("Got {} candidate servers for {}", newInstanceList.size(), name);
+            final List<ServiceInstance> newInstanceList = discoverServices();
+            log.debug("Got {} candidate servers for {}", newInstanceList.size(), getName());
             if (CollectionUtils.isEmpty(newInstanceList)) {
-                log.error("No servers found for {}", name);
-                this.savedListener.onError(Status.UNAVAILABLE.withDescription("No servers found for " + name));
+                log.error("No servers found for {}", getName());
+                this.savedListener.onError(Status.UNAVAILABLE
+                        .withDescription("No servers found for " + getName()));
                 return Lists.newArrayList();
             }
             if (!needsToUpdateConnections(newInstanceList)) {
-                log.debug("Nothing has changed... skipping update for {}", name);
+                log.debug("Nothing has changed... skipping update for {}", getName());
                 return KEEP_PREVIOUS;
             }
-            log.debug("Ready to update server list for {}", name);
+            log.debug("Ready to update server list for {}", getName());
             final List<EquivalentAddressGroup> targets = Lists.newArrayList();
             for (final ServiceInstance instance : newInstanceList) {
                 final int port = getGRPCPort(instance);
-                log.debug("Found gRPC server {}:{} for {}", instance.getHost(), port, name);
+                log.debug("Found gRPC server {}:{} for {}", instance.getHost(), port, getName());
                 targets.add(new EquivalentAddressGroup(
                         new InetSocketAddress(instance.getHost(), port), Attributes.EMPTY));
             }
             if (targets.isEmpty()) {
-                log.error("None of the servers for {} specified a gRPC port", name);
+                log.error("None of the servers for {} specified a gRPC port", getName());
                 this.savedListener.onError(Status.UNAVAILABLE
-                        .withDescription("None of the servers for " + name + " specified a gRPC port"));
+                        .withDescription("None of the servers for " + getName() + " specified a gRPC port"));
                 return Lists.newArrayList();
             } else {
                 this.savedListener.onResult(ResolutionResult.newBuilder()
                         .setAddresses(targets)
                         .build());
-                log.info("Done updating server list for {}", name);
+                log.info("Done updating server list for {}", getName());
                 return newInstanceList;
             }
         }
@@ -245,15 +270,14 @@ private int getGRPCPort(final ServiceInstance instance) {
             if (metadata == null) {
                 return instance.getPort();
             }
-            String portString = metadata.get(GrpcUtils.CLOUD_DISCOVERY_METADATA_PORT);
+            String portString = metadata.get(CLOUD_DISCOVERY_METADATA_PORT);
             if (portString == null) {
                 portString = metadata.get(LEGACY_CLOUD_DISCOVERY_METADATA_PORT);
                 if (portString == null) {
                     return instance.getPort();
                 } else {
                     log.warn("Found legacy grpc port metadata '{}' for client '{}' use '{}' instead",
-                            LEGACY_CLOUD_DISCOVERY_METADATA_PORT, DiscoveryClientNameResolver.this.name,
-                            GrpcUtils.CLOUD_DISCOVERY_METADATA_PORT);
+                            LEGACY_CLOUD_DISCOVERY_METADATA_PORT, getName(), CLOUD_DISCOVERY_METADATA_PORT);
                 }
             }
             try {
@@ -271,10 +295,10 @@ private int getGRPCPort(final ServiceInstance instance) {
          * @return True, if the given instance list contains different entries than the stored ones.
          */
         private boolean needsToUpdateConnections(final List<ServiceInstance> newInstanceList) {
-            if (this.savedInstanceList.size() != newInstanceList.size()) {
+            if (DiscoveryClientNameResolver.this.instanceList.size() != newInstanceList.size()) {
                 return true;
             }
-            for (final ServiceInstance instance : this.savedInstanceList) {
+            for (final ServiceInstance instance : DiscoveryClientNameResolver.this.instanceList) {
                 final int port = getGRPCPort(instance);
                 boolean isSame = false;
                 for (final ServiceInstance newInstance : newInstanceList) {

grpc-client-spring-boot-autoconfigure/src/main/java/net/devh/boot/grpc/client/nameresolver/DiscoveryClientResolverFactory.java

@@ -22,7 +22,6 @@
 import java.net.URI;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.atomic.AtomicReference;
 
 import javax.annotation.Nullable;
 import javax.annotation.PreDestroy;
@@ -73,12 +72,9 @@ public NameResolver newNameResolver(final URI targetUri, final NameResolver.Args
                         + "expected: '" + DISCOVERY_SCHEME + ":[//]/<service-name>'; "
                         + "but was '" + targetUri.toString() + "'");
             }
-            final AtomicReference<DiscoveryClientNameResolver> reference = new AtomicReference<>();
             final DiscoveryClientNameResolver discoveryClientNameResolver =
                     new DiscoveryClientNameResolver(serviceName.substring(1), this.client, args,
-                            GrpcUtil.SHARED_CHANNEL_EXECUTOR,
-                            () -> this.discoveryClientNameResolvers.remove(reference.get()));
-            reference.set(discoveryClientNameResolver);
+                            GrpcUtil.SHARED_CHANNEL_EXECUTOR, this.discoveryClientNameResolvers::remove);
             this.discoveryClientNameResolvers.add(discoveryClientNameResolver);
             return discoveryClientNameResolver;
         }