Merge pull request #420 from elandau/bugfix/overrides

config: Simplify how overrides are applied

ribbon-archaius/src/main/java/com/netflix/client/config/DefaultClientConfigImpl.java

@@ -17,7 +17,6 @@
  */
 package com.netflix.client.config;
 
-import com.netflix.config.ConfigurationManager;
 
 import java.util.concurrent.TimeUnit;
 
@@ -302,7 +301,6 @@ public class DefaultClientConfigImpl extends AbstractDefaultClientConfigImpl {
     }
 
     public void loadDefaultValues() {
-        super.loadDefaultValues();
         set(CommonClientConfigKey.MaxHttpConnectionsPerHost, getDefaultMaxHttpConnectionsPerHost());
         set(CommonClientConfigKey.MaxTotalHttpConnections, getDefaultMaxTotalHttpConnections());
         set(CommonClientConfigKey.EnableConnectionPool, getDefaultEnableConnectionPool());
@@ -319,18 +317,8 @@ public class DefaultClientConfigImpl extends AbstractDefaultClientConfigImpl {
         set(CommonClientConfigKey.ConnIdleEvictTimeMilliSeconds, getDefaultConnectionidleTimeInMsecs());
         set(CommonClientConfigKey.ConnectionCleanerRepeatInterval, getDefaultConnectionIdleTimertaskRepeatInMsecs());
         set(CommonClientConfigKey.EnableGZIPContentEncodingFilter, getDefaultEnableGzipContentEncodingFilter());
-        String proxyHost = ConfigurationManager.getConfigInstance().getString(getDefaultPropName(CommonClientConfigKey.ProxyHost.key()));
-        if (proxyHost != null && proxyHost.length() > 0) {
-            set(CommonClientConfigKey.ProxyHost, proxyHost);
-        }
-        Integer proxyPort = ConfigurationManager
-                .getConfigInstance()
-                .getInteger(
-                        getDefaultPropName(CommonClientConfigKey.ProxyPort),
-                        (Integer.MIN_VALUE + 1)); // + 1 just to avoid potential clash with user setting
-        if (proxyPort != (Integer.MIN_VALUE + 1)) {
-            set(CommonClientConfigKey.ProxyPort, proxyPort);
-        }
+        set(CommonClientConfigKey.ProxyHost, null);
+        set(CommonClientConfigKey.ProxyPort, null);
         set(CommonClientConfigKey.Port, getDefaultPort());
         set(CommonClientConfigKey.EnablePrimeConnections, getDefaultEnablePrimeConnections());
         set(CommonClientConfigKey.MaxRetriesPerServerPrimeConnection, getDefaultMaxRetriesPerServerPrimeConnection());

ribbon-core/src/main/java/com/netflix/client/config/CommonClientConfigKey.java

@@ -24,6 +24,7 @@ import java.lang.reflect.Modifier;
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
 import java.util.HashSet;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
@@ -285,4 +286,16 @@ public abstract class CommonClientConfigKey<T> implements IClientConfigKey<T> {
     @Override
     public T defaultValue() { return defaultValue; }
 
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        CommonClientConfigKey<?> that = (CommonClientConfigKey<?>) o;
+        return Objects.equals(configKey, that.configKey);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(configKey);
+    }
 }

ribbon-core/src/main/java/com/netflix/client/config/IClientConfig.java

@@ -49,14 +49,6 @@ public interface IClientConfig {
 
 	Map<String, Object> getProperties();
 
-    /**
-     * Iterate all properties and report the default value only to the consumer
-     * @param consumer
-     */
-    default void forEachDefault(BiConsumer<IClientConfigKey<?>, Object> consumer) {
-        throw new UnsupportedOperationException();
-    }
-
     /**
      * Iterate all properties and report the final value.  Can be null if a default value is not specified.
      * @param consumer

ribbon-core/src/main/java/com/netflix/client/config/ReloadableClientConfig.java

@@ -39,7 +39,7 @@ public abstract class ReloadableClientConfig implements IClientConfig {
     private static final String DEFAULT_NAMESPACE = "ribbon";
 
     // Map of raw property names (without namespace or client) to values set via code
-    private final Map<String, Object> internalProperties = new HashMap<>();
+    private final Map<IClientConfigKey, Object> internalProperties = new HashMap<>();
 
     // Map of all seen dynamic properties.  This map will hold on properties requested with the exception of
     // those returned from getGlobalProperty().
@@ -57,19 +57,15 @@ public abstract class ReloadableClientConfig implements IClientConfig {
 
     private String namespace = DEFAULT_NAMESPACE;
 
-    private boolean isDynamic;
-
     private boolean isLoaded = false;
 
     protected ReloadableClientConfig(PropertyResolver resolver) {
         this.clientName = DEFAULT_CLIENT_NAME;
-        this.isDynamic = false;
         this.resolver = resolver;
     }
 
     protected ReloadableClientConfig(PropertyResolver resolver, String clientName) {
         this.clientName = clientName;
-        this.isDynamic = true;
         this.resolver = resolver;
     }
 
@@ -112,29 +108,24 @@ public abstract class ReloadableClientConfig implements IClientConfig {
     public void loadProperties(String clientName) {
         Preconditions.checkState(isLoaded == false, "Config '{}' can only be loaded once", clientName);
         if (!isLoaded) {
+            loadDefaultValues();
             this.isLoaded = true;
             resolver.onChange(this::reload);
         }
 
-        this.isDynamic = true;
         this.clientName = clientName;
-        loadDefaultValues();
-    }
-
-    @Override
-    public void loadDefaultValues() {
-        this.isDynamic = true;
-        reload();
     }
 
     @Override
     public final Map<String, Object> getProperties() {
-        return Collections.unmodifiableMap(internalProperties);
-    }
-
-    @Override
-    public void forEachDefault(BiConsumer<IClientConfigKey<?>, Object> consumer) {
-        dynamicProperties.forEach((key, value) -> consumer.accept(key, internalProperties.get(key.key())));
+        final Map<String, Object> result = new HashMap<>(dynamicProperties.size());
+        dynamicProperties.forEach((key, value) -> {
+            Object v = value.get().orElse(null);
+            if (v != null) {
+                result.put(key.key(), String.valueOf(v));
+            }
+        });
+        return result;
     }
 
     @Override
@@ -150,9 +141,7 @@ public abstract class ReloadableClientConfig implements IClientConfig {
 
             {
                 refresh();
-                if (isDynamic) {
-                    changeActions.add(this::refresh);
-                }
+                changeActions.add(this::refresh);
             }
 
             @Override
@@ -192,11 +181,7 @@ public abstract class ReloadableClientConfig implements IClientConfig {
 
     @Override
     public final <T> T get(IClientConfigKey<T> key) {
-        if (isLoaded) {
-            return getOrCreateProperty(key).get().orElse(null);
-        } else {
-            return getIfSet(key).orElse(null);
-        }
+        return getOrCreateProperty(key).get().orElse(null);
     }
 
     private final <T> ReloadableProperty<T> getOrCreateProperty(IClientConfigKey<T> key) {
@@ -251,6 +236,12 @@ public abstract class ReloadableClientConfig implements IClientConfig {
         return getIfSet(key);
     }
 
+    /**
+     * Resolve a p
+     * @param key
+     * @param <T>
+     * @return
+     */
     private <T> Optional<T> resolverScopedProperty(IClientConfigKey<T> key) {
         Optional<T> value = resolver.get(clientName + "." + getNameSpace() + "." + key.key(), key.type());
         if (value.isPresent()) {
@@ -262,7 +253,7 @@ public abstract class ReloadableClientConfig implements IClientConfig {
 
     @Override
     public <T> Optional<T> getIfSet(IClientConfigKey<T> key) {
-        return Optional.ofNullable(internalProperties.get(key.key()))
+        return Optional.ofNullable(internalProperties.get(key))
                 .map(value -> {
                     final Class<T> type = key.type();
                     // Unfortunately there's some legacy code setting string values for typed keys.  Here are do our best to parse
@@ -359,7 +350,7 @@ public abstract class ReloadableClientConfig implements IClientConfig {
         if (value == null) {
             internalProperties.remove(key.key());
         } else {
-            internalProperties.put(key.key(), value);
+            internalProperties.put(key, value);
         }
 
         if (isLoaded) {
@@ -418,8 +409,16 @@ public abstract class ReloadableClientConfig implements IClientConfig {
             return this;
         }
 
-        this.internalProperties.putAll(override.getProperties());
-        reload();
+        // When overriding we only really care of picking up properties that were explicitly set in code.  This is a
+        // bit of an optimization to avoid excessive memory allocation as requests are made and overrides are applied
+        if (override instanceof ReloadableClientConfig) {
+            ((ReloadableClientConfig)override).internalProperties.forEach((key, value) -> {
+                if (value != null) {
+                    setProperty(key, value);
+                }
+            });
+        }
+
         return this;
     }
 

ribbon-core/src/test/java/com/netflix/client/config/ClientConfigTest.java

@@ -30,6 +30,8 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TestName;
 import org.junit.runners.MethodSorters;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.util.Properties;
 
@@ -41,6 +43,8 @@ import java.util.Properties;
  */
 @FixMethodOrder(MethodSorters.NAME_ASCENDING)
 public class ClientConfigTest {
+    private static final Logger LOG = LoggerFactory.getLogger(ClientConfigTest.class);
+
     @Rule
     public TestName testName = new TestName();
 
@@ -186,7 +190,7 @@ public class ClientConfigTest {
         ConfigurationManager.getConfigInstance().setProperty("testValueOf.ribbon.CustomValueOf", "value");
 
         DefaultClientConfigImpl clientConfig = new DefaultClientConfigImpl();
-        clientConfig.setClientName("testValueOf");
+        clientConfig.loadProperties("testValueOf");
 
         Property<CustomValueOf> prop = clientConfig.getDynamicProperty(CUSTOM_KEY);
         Assert.assertEquals("value", prop.getOrDefault().getValue());
@@ -198,7 +202,7 @@ public class ClientConfigTest {
         ConfigurationManager.getConfigInstance().setProperty("testScopedProperty.ribbon.foo.ScopePropertyTimeout", "1000");
 
         DefaultClientConfigImpl clientConfig = new DefaultClientConfigImpl();
-        clientConfig.setClientName("testScopedProperty");
+        clientConfig.loadProperties("testScopedProperty");
 
         Property<Integer> prop = clientConfig.getScopedProperty(new CommonClientConfigKey<Integer>("foo.ScopePropertyTimeout", 0) {});
         Assert.assertEquals(1000, prop.get().get().intValue());

ribbon-loadbalancer/src/test/java/com/netflix/loadbalancer/SubsetFilterTest.java

@@ -84,7 +84,7 @@ public class SubsetFilterTest {
     @Test
     public void testFiltering() {
         DefaultClientConfigImpl config = new DefaultClientConfigImpl();
-        config.setClientName("SubsetFilerTest");
+        config.loadProperties("SubsetFilerTest");
 
         ServerListSubsetFilter<Server> filter = new ServerListSubsetFilter<Server>(config);
         LoadBalancerStats stats = new LoadBalancerStats("default");

ribbon-loadbalancer/src/test/java/com/netflix/loadbalancer/ZoneAwareLoadBalancerTest.java

@@ -74,7 +74,7 @@ public class ZoneAwareLoadBalancerTest {
         ConfigurationManager.getConfigInstance().setProperty("niws.loadbalancer.serverStats.activeRequestsCount.effectiveWindowSeconds", 10);
 
         DefaultClientConfigImpl config = new DefaultClientConfigImpl();
-        config.setClientName("testChooseZone");
+        config.loadProperties("testChooseZone");
         ZoneAwareLoadBalancer<Server> balancer = new ZoneAwareLoadBalancer<Server>();
         balancer.init();
         IRule globalRule = new RoundRobinRule();