Merge pull request #1930 from yahoo/hmusum/refactor-config-proxy-part-3

Refactorings

6 files changed, 67 insertions, 81 deletions

diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ConfigSourceClient.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ConfigSourceClient.java
index f8b2cafa50f..b805640de87 100644
--- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ConfigSourceClient.java
+++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ConfigSourceClient.java
@@ -1,10 +1,7 @@
 // Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.proxy;
 
-import com.yahoo.config.subscription.ConfigSource;
-import com.yahoo.config.subscription.ConfigSourceSet;
 import com.yahoo.vespa.config.RawConfig;
-import com.yahoo.vespa.config.TimingValues;
 import com.yahoo.vespa.config.protocol.JRTServerConfigRequest;
 
 import java.util.List;
@@ -16,29 +13,15 @@ import java.util.List;
  * @author hmusum
  * @since 5.1.9
  */
-public abstract class ConfigSourceClient {
-    static ConfigSourceClient createClient(ConfigSource source,
-                                           ClientUpdater clientUpdater,
-                                           MemoryCache memoryCache,
-                                           TimingValues timingValues,
-                                           DelayedResponses delayedResponses) {
-        if (source instanceof ConfigSourceSet) {
-            return new RpcConfigSourceClient((ConfigSourceSet) source, clientUpdater, memoryCache, timingValues, delayedResponses);
-        } else if (source instanceof MapBackedConfigSource) {
-            return (ConfigSourceClient) source;
-        } else {
-            throw new IllegalArgumentException("config source of type " + source.getClass().getName() + " is not allowed");
-        }
-    }
-
-    abstract RawConfig getConfig(RawConfig input, JRTServerConfigRequest request);
-
-    abstract void cancel();
-
-    // TODO Should only be in rpc config source client
-    abstract void shutdownSourceConnections();
-
-    abstract String getActiveSourceConnection();
-
-    abstract List<String> getSourceConnections();
+interface ConfigSourceClient {
+
+    RawConfig getConfig(RawConfig input, JRTServerConfigRequest request);
+
+    void cancel();
+
+    void shutdownSourceConnections();
+
+    String getActiveSourceConnection();
+
+    List<String> getSourceConnections();
 }
\ No newline at end of file
diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MapBackedConfigSource.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MapBackedConfigSource.java
index 164b486334c..e8a76257b14 100644
--- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MapBackedConfigSource.java
+++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MapBackedConfigSource.java
@@ -17,7 +17,7 @@ import java.util.List;
  * @author hmusum
  * @since 5.1.10
  */
-public class MapBackedConfigSource extends ConfigSourceClient implements ConfigSource {
+public class MapBackedConfigSource implements ConfigSource, ConfigSourceClient {
     private final HashMap<ConfigKey<?>, RawConfig> backing = new HashMap<>();
     private final ClientUpdater clientUpdater;
 
@@ -32,7 +32,7 @@ public class MapBackedConfigSource extends ConfigSourceClient implements ConfigS
     }
 
     @Override
-    RawConfig getConfig(RawConfig input, JRTServerConfigRequest request) {
+    public RawConfig getConfig(RawConfig input, JRTServerConfigRequest request) {
         return getConfig(input.getKey());
     }
 
@@ -41,12 +41,12 @@ public class MapBackedConfigSource extends ConfigSourceClient implements ConfigS
     }
 
     @Override
-    void cancel() {
+    public void cancel() {
         clear();
     }
 
     @Override
-    void shutdownSourceConnections() {
+    public void shutdownSourceConnections() {
     }
 
     public void clear() {
@@ -54,12 +54,12 @@ public class MapBackedConfigSource extends ConfigSourceClient implements ConfigS
     }
 
     @Override
-    String getActiveSourceConnection() {
+    public String getActiveSourceConnection() {
         return "N/A";
     }
 
     @Override
-    List<String> getSourceConnections() {
+    public List<String> getSourceConnections() {
         return Collections.singletonList("N/A");
     }
 }
diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MemoryCacheConfigClient.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MemoryCacheConfigClient.java
index f1980b05817..6caed8a77e6 100644
--- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MemoryCacheConfigClient.java
+++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MemoryCacheConfigClient.java
@@ -13,7 +13,7 @@ import java.util.logging.Logger;
  * @author hmusum
  * @since 5.1.10
  */
-class MemoryCacheConfigClient extends ConfigSourceClient {
+class MemoryCacheConfigClient implements ConfigSourceClient {
 
     private final static Logger log = Logger.getLogger(MemoryCacheConfigClient.class.getName());
     private final MemoryCache cache;
@@ -29,7 +29,7 @@ class MemoryCacheConfigClient extends ConfigSourceClient {
      * @return A Config with a payload.
      */
     @Override
-    RawConfig getConfig(RawConfig input, JRTServerConfigRequest request) {
+    public RawConfig getConfig(RawConfig input, JRTServerConfigRequest request) {
         log.log(LogLevel.DEBUG, "Getting config from cache");
         ConfigKey<?> key = input.getKey();
         RawConfig cached = cache.get(new ConfigCacheKey(key, input.getDefMd5()));
@@ -42,20 +42,20 @@ class MemoryCacheConfigClient extends ConfigSourceClient {
     }
 
     @Override
-    void cancel() {
+    public void cancel() {
     }
 
     @Override
-    void shutdownSourceConnections() {
+    public void shutdownSourceConnections() {
     }
 
     @Override
-    String getActiveSourceConnection() {
+    public String getActiveSourceConnection() {
         return "N/A";
     }
 
     @Override
-    List<String> getSourceConnections() {
+    public List<String> getSourceConnections() {
         return Collections.singletonList("N/A");
     }
 
diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ProxyServer.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ProxyServer.java
index a0ce5853768..c5df12f1e96 100644
--- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ProxyServer.java
+++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ProxyServer.java
@@ -24,7 +24,7 @@ import static com.yahoo.vespa.config.proxy.Mode.ModeName.DEFAULT;
 import static java.util.concurrent.TimeUnit.SECONDS;
 
 /**
- * A proxy server that handles RPC config requests. The proxy can run in three modes:
+ * A proxy server that handles RPC config requests. The proxy can run in two modes:
  * 'default' and 'memorycache', where the last one will not get config from an upstream
  * config source, but will serve config only from memory cache.
  *
@@ -47,7 +47,7 @@ public class ProxyServer implements Runnable {
     final DelayedResponses delayedResponses;
     private ConfigSource configSource;
 
-    private volatile ConfigSourceClient configSourceClient;
+    private volatile ConfigSourceClient configClient;
 
     private final ConfigProxyStatistics statistics;
     private final TimingValues timingValues;
@@ -56,7 +56,7 @@ public class ProxyServer implements Runnable {
     private final static TimingValues defaultTimingValues;
     private final boolean delayedResponseHandling;
 
-    private volatile Mode mode;
+    private volatile Mode mode = new Mode(DEFAULT);
 
     static {
         // Proxy should time out before clients upon subscription.
@@ -69,38 +69,35 @@ public class ProxyServer implements Runnable {
     }
 
     private ProxyServer(Spec spec, DelayedResponses delayedResponses, ConfigSource source,
-                       ConfigProxyStatistics statistics, TimingValues timingValues, Mode mode, boolean delayedResponseHandling,
-                       MemoryCache memoryCache) {
+                        ConfigProxyStatistics statistics, TimingValues timingValues,
+                        boolean delayedResponseHandling, MemoryCache memoryCache,
+                        ConfigSourceClient configClient) {
         this.delayedResponses = delayedResponses;
         this.configSource = source;
         log.log(LogLevel.DEBUG, "Using config source '" + source);
         this.statistics = statistics;
         this.timingValues = timingValues;
-        this.mode = mode;
         this.delayedResponseHandling = delayedResponseHandling;
         this.memoryCache = memoryCache;
-        if (spec == null) {
-            rpcServer = null;
-        } else {
-            rpcServer = new ConfigProxyRpcServer(this, spec);
-        }
+        this.rpcServer = createRpcServer(spec);
         clientUpdater = new ClientUpdater(memoryCache, rpcServer, statistics, delayedResponses, mode);
-        this.configSourceClient = ConfigSourceClient.createClient(source, clientUpdater, memoryCache, timingValues, delayedResponses);
+        this.configClient = createClient(clientUpdater, delayedResponses, source, timingValues, memoryCache, configClient);
     }
 
-    private static ProxyServer create(int port, DelayedResponses delayedResponses, ConfigSource source,
-                                      ConfigProxyStatistics statistics, Mode mode) {
-        return new ProxyServer(new Spec(null, port), delayedResponses, source, statistics, defaultTimingValues(), mode, true, new MemoryCache());
+    static ProxyServer createTestServer(ConfigSourceSet source) {
+        return createTestServer(source, null);
     }
 
+    static ProxyServer createTestServer(MapBackedConfigSource source) {
+        return createTestServer(source, source);
+    }
 
-    static ProxyServer createTestServer(ConfigSource source) {
-        final Mode mode = new Mode(DEFAULT);
+    private static ProxyServer createTestServer(ConfigSource source, ConfigSourceClient configSourceClient) {
         final ConfigProxyStatistics statistics = new ConfigProxyStatistics();
         final boolean delayedResponseHandling = false;
         return new ProxyServer(null, new DelayedResponses(statistics),
-                               source, statistics, defaultTimingValues(), mode, delayedResponseHandling,
-                               new MemoryCache());
+                               source, statistics, defaultTimingValues(), delayedResponseHandling,
+                               new MemoryCache(), configSourceClient);
     }
 
     public void run() {
@@ -126,7 +123,7 @@ public class ProxyServer implements Runnable {
         // create a background thread that retrieves config from the server and
         // calls updateSubscribers when new config is returned from the config source.
         // In the last case the method below will return null.
-        RawConfig config = configSourceClient.getConfig(RawConfig.createFromServerRequest(req), req);
+        RawConfig config = configClient.getConfig(RawConfig.createFromServerRequest(req), req);
         if (configOrGenerationHasChanged(config, req)) {
             memoryCache.put(config);
         }
@@ -148,18 +145,30 @@ public class ProxyServer implements Runnable {
         this.mode = new Mode(modeName);
         switch (mode.getMode()) {
             case MEMORYCACHE:
-                configSourceClient.shutdownSourceConnections();
-                configSourceClient = new MemoryCacheConfigClient(memoryCache);
+                configClient.shutdownSourceConnections();
+                configClient = new MemoryCacheConfigClient(memoryCache);
                 break;
             case DEFAULT:
                 flush();
-                configSourceClient = createRpcClient();
+                configClient = createRpcClient();
                 break;
             default:
                 throw new IllegalArgumentException("Not able to handle mode '" + modeName + "'");
         }
     }
 
+    private ConfigSourceClient createClient(ClientUpdater clientUpdater, DelayedResponses delayedResponses,
+                                            Object source, TimingValues timingValues,
+                                            MemoryCache memoryCache, ConfigSourceClient client) {
+        return (client == null)
+                ? new RpcConfigSourceClient((ConfigSourceSet) source, clientUpdater, memoryCache, timingValues, delayedResponses)
+                : client;
+    }
+
+    private ConfigProxyRpcServer createRpcServer(Spec spec) {
+        return  (spec == null) ? null : new ConfigProxyRpcServer(this, spec); // TODO: Try to avoid first argument being 'this'
+    }
+
     private RpcConfigSourceClient createRpcClient() {
         return new RpcConfigSourceClient((ConfigSourceSet) configSource, clientUpdater, memoryCache, timingValues, delayedResponses);
     }
@@ -202,7 +211,8 @@ public class ProxyServer implements Runnable {
 
         ConfigSourceSet configSources = new ConfigSourceSet(properties.configSources);
         DelayedResponses delayedResponses = new DelayedResponses(statistics);
-        ProxyServer proxyServer = ProxyServer.create(port, delayedResponses, configSources, statistics, new Mode(DEFAULT));
+        ProxyServer proxyServer = new ProxyServer(new Spec(null, port), delayedResponses, configSources, statistics,
+                                                  defaultTimingValues(), true, new MemoryCache(), null);
         // catch termination signal
         proxyServer.setupSigTermHandler();
         Thread proxyserverThread = new Thread(proxyServer);
@@ -244,7 +254,7 @@ public class ProxyServer implements Runnable {
     // the cache will not be updated again before someone calls getConfig().
     synchronized void flush() {
         memoryCache.clear();
-        configSourceClient.cancel();
+        configClient.cancel();
     }
 
     public void stop() {
@@ -262,16 +272,16 @@ public class ProxyServer implements Runnable {
     }
 
     String getActiveSourceConnection() {
-        return configSourceClient.getActiveSourceConnection();
+        return configClient.getActiveSourceConnection();
     }
 
     List<String> getSourceConnections() {
-        return configSourceClient.getSourceConnections();
+        return configClient.getSourceConnections();
     }
 
     public void updateSourceConnections(List<String> sources) {
         configSource = new ConfigSourceSet(sources);
         flush();
-        configSourceClient = createRpcClient();
+        configClient = createRpcClient();
     }
 }
diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/RpcConfigSourceClient.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/RpcConfigSourceClient.java
index 942d06367b8..b0ad57e8a6c 100644
--- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/RpcConfigSourceClient.java
+++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/RpcConfigSourceClient.java
@@ -25,7 +25,7 @@ import java.util.logging.Logger;
  * @author hmusum
  * @since 5.1.9
  */
-class RpcConfigSourceClient extends ConfigSourceClient {
+class RpcConfigSourceClient implements ConfigSourceClient {
 
     private final static Logger log = Logger.getLogger(RpcConfigSourceClient.class.getName());
     private final Supervisor supervisor = new Supervisor(new Transport());
@@ -111,7 +111,7 @@ class RpcConfigSourceClient extends ConfigSourceClient {
      * @return A Config with a payload.
      */
     @Override
-    RawConfig getConfig(RawConfig input, JRTServerConfigRequest request) {
+    public RawConfig getConfig(RawConfig input, JRTServerConfigRequest request) {
         long start = System.currentTimeMillis();
         RawConfig ret = null;
         final ConfigCacheKey configCacheKey = new ConfigCacheKey(input.getKey(), input.getDefMd5());
@@ -166,7 +166,7 @@ class RpcConfigSourceClient extends ConfigSourceClient {
     }
 
     @Override
-    void cancel() {
+    public void cancel() {
         shutdownSourceConnections();
     }
 
@@ -174,7 +174,7 @@ class RpcConfigSourceClient extends ConfigSourceClient {
      * Takes down connection(s) to config sources and running tasks
      */
     @Override
-    void shutdownSourceConnections() {
+    public void shutdownSourceConnections() {
         synchronized (activeSubscribersLock) {
             for (Subscriber subscriber : activeSubscribers.values()) {
                 subscriber.cancel();
@@ -188,7 +188,7 @@ class RpcConfigSourceClient extends ConfigSourceClient {
     }
 
     @Override
-    String getActiveSourceConnection() {
+    public String getActiveSourceConnection() {
         if (requesterPool.get(configSourceSet) != null) {
             return requesterPool.get(configSourceSet).getConnectionPool().getCurrent().getAddress();
         } else {
@@ -197,7 +197,7 @@ class RpcConfigSourceClient extends ConfigSourceClient {
     }
 
     @Override
-    List<String> getSourceConnections() {
+    public List<String> getSourceConnections() {
         ArrayList<String> ret = new ArrayList<>();
         final JRTConfigRequester jrtConfigRequester = requesterPool.get(configSourceSet);
         if (jrtConfigRequester != null) {
diff --git a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/ProxyServerTest.java b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/ProxyServerTest.java
index bdeedba7b78..ed6d2056733 100644
--- a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/ProxyServerTest.java
+++ b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/ProxyServerTest.java
@@ -2,7 +2,6 @@
 package com.yahoo.vespa.config.proxy;
 
 import com.yahoo.config.subscription.ConfigSourceSet;
-import com.yahoo.config.subscription.RawSource;
 import com.yahoo.vespa.config.*;
 import com.yahoo.vespa.config.protocol.JRTServerConfigRequest;
 import org.junit.After;
@@ -168,12 +167,6 @@ public class ProxyServerTest {
         assertThat(properties.configSources[0], is(ProxyServer.DEFAULT_PROXY_CONFIG_SOURCES));
     }
 
-    @Test(expected = IllegalArgumentException.class)
-    public void testIllegalConfigSource() {
-        RawSource source = new RawSource("bar 1");
-        proxy = ProxyServer.createTestServer(source);
-    }
-
     static RawConfig createConfigWithNextConfigGeneration(RawConfig config, int errorCode) {
         return new RawConfig(config.getKey(), config.getDefMd5(),
             config.getPayload(), config.getConfigMd5(),