diff --git a/ribbon-core/src/main/java/com/netflix/client/config/ReloadableClientConfig.java b/ribbon-core/src/main/java/com/netflix/client/config/ReloadableClientConfig.java index eb16a9fbb60e524e63ad43a52c0333e24d9c6b00..e53ecd82af5b149987d08b14a33941ef5e2120eb 100644 --- a/ribbon-core/src/main/java/com/netflix/client/config/ReloadableClientConfig.java +++ b/ribbon-core/src/main/java/com/netflix/client/config/ReloadableClientConfig.java @@ -38,16 +38,15 @@ public abstract class ReloadableClientConfig implements IClientConfig { private static final String DEFAULT_CLIENT_NAME = ""; private static final String DEFAULT_NAMESPACE = "ribbon"; - // Map of raw property names (without namespace or client) to values set via code - private final Map internalProperties = new HashMap<>(); + // Map of raw property names (without namespace or client name) to values. All values are non-null and properly + // typed to match the key type + private final Map> internalProperties = new ConcurrentHashMap<>(); - // Map of all seen dynamic properties. This map will hold on properties requested with the exception of - // those returned from getGlobalProperty(). private final Map> dynamicProperties = new ConcurrentHashMap<>(); // List of actions to perform when configuration changes. This includes both updating the Property instances // as well as external consumers. - private final List changeActions = new CopyOnWriteArrayList<>(); + private final Map changeActions = new ConcurrentHashMap<>(); private final AtomicLong refreshCounter = new AtomicLong(); @@ -89,7 +88,8 @@ public abstract class ReloadableClientConfig implements IClientConfig { * Refresh all seen properties from the underlying property storage */ public final void reload() { - changeActions.forEach(Runnable::run); + changeActions.values().forEach(Runnable::run); + dynamicProperties.values().forEach(ReloadableProperty::reload); cachedToString = null; } @@ -125,94 +125,148 @@ public abstract class ReloadableClientConfig implements IClientConfig { resolver.onChange(this::reload); } + /** + * @return use {@link #forEach(BiConsumer)} + */ @Override + @Deprecated public final Map getProperties() { - final Map 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)); - } - }); + final Map result = new HashMap<>(internalProperties.size()); + forEach((key, value) -> result.put(key.key(), String.valueOf(value))); return result; } @Override public void forEach(BiConsumer, Object> consumer) { - dynamicProperties.forEach((key, value) -> consumer.accept(key, value.get().orElse(null))); + internalProperties.forEach((key, value) -> { + if (value.isPresent()) { + consumer.accept(key, value.get()); + } + }); } - private ReloadableProperty createProperty(final Supplier> valueSupplier, final Supplier defaultSupplier) { + /** + * Create an action that will refresh the cached value for key. Uses the current value as a reference and will + * update from the dynamic property source to either delete or set a new value. + * + * @param key - Property key without client name or namespace + * @param valueSupplier - Strategy for generating the value. Could potentially ready multiple property names, such + * as default and client specific + */ + private Runnable createAutoRefreshAction(final IClientConfigKey key, final Supplier> valueSupplier) { + final AtomicReference> current = new AtomicReference<>(valueSupplier.get()); + return () -> { + final Optional next = valueSupplier.get(); + if (!next.equals(current.get())) { + LOG.debug("New value {}: {} -> {}", key.key(), current.get(), next); + current.set(next); + if (next.isPresent()) { + internalProperties.put(key, (Optional) next); + } else { + internalProperties.put(key, Optional.empty()); + } + } + }; + } + + interface ReloadableProperty extends Property { + void reload(); + } + + private synchronized Property getOrCreateProperty(final IClientConfigKey key, final Supplier> valueSupplier, final Supplier defaultSupplier) { Preconditions.checkNotNull(valueSupplier, "defaultValueSupplier cannot be null"); - return new ReloadableProperty() { - private volatile Optional value = Optional.empty(); + return (Property)dynamicProperties.computeIfAbsent(key, ignore -> new ReloadableProperty() { + private volatile Optional current = Optional.empty(); + private List> consumers = new CopyOnWriteArrayList<>(); - { - refresh(); - changeActions.add(this::refresh); - } + { + reload(); + } - @Override - public void onChange(Consumer consumer) { - final AtomicReference> previous = new AtomicReference<>(get()); - changeActions.add(() -> { - Optional current = get(); - if (!current.equals(Optional.ofNullable(previous.get()))) { - previous.set(current); - consumer.accept(current.orElseGet(defaultSupplier::get)); - } - }); - } + @Override + public void onChange(Consumer consumer) { + consumers.add(consumer); + } - @Override - public Optional get() { - return value; - } + @Override + public Optional get() { + return current; + } - @Override - public T getOrDefault() { - return value.orElse(defaultSupplier.get()); - } + @Override + public T getOrDefault() { + return current.orElse(defaultSupplier.get()); + } - @Override - public void refresh() { - refreshCounter.incrementAndGet(); - value = valueSupplier.get(); - } + @Override + public void reload() { + refreshCounter.incrementAndGet(); - @Override - public String toString() { - return String.valueOf(get()); - } - }; + Optional next = valueSupplier.get(); + if (!next.equals(current)) { + current = next; + consumers.forEach(consumer -> consumer.accept(next.orElseGet(defaultSupplier::get))); + } + } + + @Override + public String toString() { + return String.valueOf(get()); + } + }); } @Override public final T get(IClientConfigKey key) { - return getOrCreateProperty(key).get().orElse(null); - } + Optional value = (Optional)internalProperties.get(key); + if (value == null) { + set(key, null); + value = (Optional)internalProperties.get(key); + } - private final ReloadableProperty getOrCreateProperty(IClientConfigKey key) { - return (ReloadableProperty) dynamicProperties.computeIfAbsent(key, ignore -> getClientDynamicProperty(key)); + return value.orElse(null); } @Override public final Property getGlobalProperty(IClientConfigKey key) { LOG.debug("Get global property {} default {}", key.key(), key.defaultValue()); - return (Property) dynamicProperties.computeIfAbsent(key, ignore -> createProperty( + return getOrCreateProperty( + key, () -> resolver.get(key.key(), key.type()), - key::defaultValue)); + key::defaultValue); } - interface ReloadableProperty extends Property { - void refresh(); + @Override + public final Property getDynamicProperty(IClientConfigKey key) { + LOG.debug("Get dynamic property key={} ns={} client={}", key.key(), getNameSpace(), clientName); + + get(key); + + return getOrCreateProperty( + key, + () -> (Optional)internalProperties.get(key), + key::defaultValue); } - private ReloadableProperty getClientDynamicProperty(IClientConfigKey key) { - return createProperty( - () -> resolveFinalProperty(key), + @Override + public Property getScopedProperty(IClientConfigKey key) { + LOG.debug("Get dynamic property key={} ns={} client={}", key.key(), getNameSpace(), clientName); + + return getOrCreateProperty( + key, + () -> resolverScopedProperty(key), + key::defaultValue); + } + + @Override + public Property getPrefixMappedProperty(IClientConfigKey key) { + LOG.debug("Get dynamic property key={} ns={} client={}", key.key(), getNameSpace(), clientName); + + return getOrCreateProperty( + key, + getPrefixedMapPropertySupplier(key), key::defaultValue); } @@ -235,12 +289,7 @@ public abstract class ReloadableClientConfig implements IClientConfig { } } - value = resolver.get(getNameSpace() + "." + key.key(), key.type()); - if (value.isPresent()) { - return value; - } - - return getIfSet(key); + return resolver.get(getNameSpace() + "." + key.key(), key.type()); } /** @@ -260,69 +309,49 @@ public abstract class ReloadableClientConfig implements IClientConfig { @Override public Optional getIfSet(IClientConfigKey key) { - return Optional.ofNullable(internalProperties.get(key)) - .map(value -> { - final Class type = key.type(); - // Unfortunately there's some legacy code setting string values for typed keys. Here are do our best to parse - // and store the typed value - if (!value.getClass().equals(type)) { - try { - if (type.equals(String.class)) { - return (T) value.toString(); - } else if (value.getClass().equals(String.class)) { - final String strValue = (String) value; - if (Integer.class.equals(type)) { - return (T) Integer.valueOf(strValue); - } else if (Boolean.class.equals(type)) { - return (T) Boolean.valueOf(strValue); - } else if (Float.class.equals(type)) { - return (T) Float.valueOf(strValue); - } else if (Long.class.equals(type)) { - return (T) Long.valueOf(strValue); - } else if (Double.class.equals(type)) { - return (T) Double.valueOf(strValue); - } else if (TimeUnit.class.equals(type)) { - return (T) TimeUnit.valueOf(strValue); - } else { - return PropertyResolver.resolveWithValueOf(type, strValue) - .orElseThrow(() -> new IllegalArgumentException("Unsupported value type `" + type + "'")); - } - } else { - return PropertyResolver.resolveWithValueOf(type, value.toString()) - .orElseThrow(() -> new IllegalArgumentException("Incompatible value type `" + value.getClass() + "` while expecting '" + type + "`")); - } - } catch (Exception e) { - throw new IllegalArgumentException("Error parsing value '" + value + "' for '" + key.key() + "'", e); - } - } else { - return (T)value; - } - }); - } - - @Override - public final Property getDynamicProperty(IClientConfigKey key) { - LOG.debug("Get dynamic property key={} ns={} client={}", key.key(), getNameSpace(), clientName); - - return getClientDynamicProperty(key); - } - - @Override - public Property getScopedProperty(IClientConfigKey key) { - LOG.debug("Get dynamic property key={} ns={} client={}", key.key(), getNameSpace(), clientName); - - return (Property) dynamicProperties.computeIfAbsent(key, ignore -> createProperty( - () -> resolverScopedProperty(key), - key::defaultValue)); + return Optional.ofNullable((T)internalProperties.get(key)); } - @Override - public Property getPrefixMappedProperty(IClientConfigKey key) { - LOG.debug("Get dynamic property key={} ns={} client={}", key.key(), getNameSpace(), clientName); + private T resolveValueToType(IClientConfigKey key, Object value) { + if (value == null) { + return null; + } - return (Property) dynamicProperties.computeIfAbsent(key, ignore -> createProperty( - getPrefixedMapPropertySupplier(key), - key::defaultValue)); + final Class type = key.type(); + // Unfortunately there's some legacy code setting string values for typed keys. Here are do our best to parse + // and store the typed value + if (!value.getClass().equals(type)) { + try { + if (type.equals(String.class)) { + return (T) value.toString(); + } else if (value.getClass().equals(String.class)) { + final String strValue = (String) value; + if (Integer.class.equals(type)) { + return (T) Integer.valueOf(strValue); + } else if (Boolean.class.equals(type)) { + return (T) Boolean.valueOf(strValue); + } else if (Float.class.equals(type)) { + return (T) Float.valueOf(strValue); + } else if (Long.class.equals(type)) { + return (T) Long.valueOf(strValue); + } else if (Double.class.equals(type)) { + return (T) Double.valueOf(strValue); + } else if (TimeUnit.class.equals(type)) { + return (T) TimeUnit.valueOf(strValue); + } else { + return PropertyResolver.resolveWithValueOf(type, strValue) + .orElseThrow(() -> new IllegalArgumentException("Unsupported value type `" + type + "'")); + } + } else { + return PropertyResolver.resolveWithValueOf(type, value.toString()) + .orElseThrow(() -> new IllegalArgumentException("Incompatible value type `" + value.getClass() + "` while expecting '" + type + "`")); + } + } catch (Exception e) { + throw new IllegalArgumentException("Error parsing value '" + value + "' for '" + key.key() + "'", e); + } + } else { + return (T)value; + } } private Supplier> getPrefixedMapPropertySupplier(IClientConfigKey key) { @@ -359,15 +388,23 @@ public abstract class ReloadableClientConfig implements IClientConfig { @Override public final IClientConfig set(IClientConfigKey key, T value) { Preconditions.checkArgument(key != null, "key cannot be null"); - // Treat nulls as deletes - if (value == null) { - internalProperties.remove(key.key()); - } else { - internalProperties.put(key, value); + + // Make sure the value is property typed + value = resolveValueToType(key, value); + + // If a client is already specified then check if there's a dynamic config override available + if (isLoaded) { + value = resolveFinalProperty(key).orElse(value); } + // Cache this new state + internalProperties.put(key, Optional.ofNullable(value)); + + // If this is the first time a property is seen and a client name has been specified then make sure + // we have the necessary refresh to pick up new values from dynamic config if (isLoaded) { - getOrCreateProperty(key).refresh(); + changeActions.computeIfAbsent(key, ignore -> createAutoRefreshAction(key, () -> resolveFinalProperty(key))) + .run(); } cachedToString = null; @@ -390,13 +427,13 @@ public abstract class ReloadableClientConfig implements IClientConfig { @Override @Deprecated public Object getProperty(IClientConfigKey key, Object defaultVal) { - return getOrCreateProperty(key).get().orElse(defaultVal); + return Optional.ofNullable(get(key)).orElse(defaultVal); } @Override @Deprecated public boolean containsProperty(IClientConfigKey key) { - return dynamicProperties.containsKey(key.key()); + return internalProperties.containsKey(key); } @Override @@ -422,15 +459,7 @@ public abstract class ReloadableClientConfig implements IClientConfig { return this; } - // 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); - } - }); - } + override.forEach((key, value) -> setProperty(key, value)); return this; } @@ -455,14 +484,12 @@ public abstract class ReloadableClientConfig implements IClientConfig { } private String generateToString() { - return "ClientConfig:" + dynamicProperties.entrySet().stream() + return "ClientConfig:" + internalProperties.entrySet().stream() .map(t -> { if (t.getKey().key().endsWith("Password")) { return t.getKey() + ":***"; } - Optional value = t.getValue().get(); - Object defaultValue = t.getKey().defaultValue(); - return t.getKey() + ":" + value.orElse(defaultValue); + return t.getKey() + ":" + t.getValue(); }) .collect(Collectors.joining(", ")); } diff --git a/ribbon-core/src/test/java/com/netflix/client/config/ClientConfigTest.java b/ribbon-core/src/test/java/com/netflix/client/config/ClientConfigTest.java index 2cf35432698075443e741eec6d573f3e2e3864ea..a233195d076116e7e594b6b52a19d4148dbe13e3 100644 --- a/ribbon-core/src/test/java/com/netflix/client/config/ClientConfigTest.java +++ b/ribbon-core/src/test/java/com/netflix/client/config/ClientConfigTest.java @@ -33,6 +33,7 @@ import org.junit.runners.MethodSorters; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.util.Objects; import java.util.Properties; /** @@ -123,9 +124,10 @@ public class ClientConfigTest { ConfigurationManager.getConfigInstance().setProperty("testRestClient2.ribbon.DeploymentContextBasedVipAddresses", "movieservice-xbox-test:7001"); assertEquals("movieservice-xbox-test:7001", clientConfig.get(CommonClientConfigKey.DeploymentContextBasedVipAddresses)); - + ConfigurationManager.getConfigInstance().clearProperty("testRestClient2.ribbon.EnableZoneAffinity"); - assertFalse(clientConfig.get(CommonClientConfigKey.EnableZoneAffinity)); + assertNull(clientConfig.get(CommonClientConfigKey.EnableZoneAffinity)); + assertFalse(clientConfig.getOrDefault(CommonClientConfigKey.EnableZoneAffinity)); } @Test @@ -173,6 +175,24 @@ public class ClientConfigTest { public String getValue() { return value; } + + @Override + public String toString() { + return value; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + CustomValueOf that = (CustomValueOf) o; + return Objects.equals(value, that.value); + } + + @Override + public int hashCode() { + return Objects.hash(value); + } } public static IClientConfigKey CUSTOM_KEY = new CommonClientConfigKey("CustomValueOf", new CustomValueOf("default")) {}; @@ -194,6 +214,9 @@ public class ClientConfigTest { Property prop = clientConfig.getDynamicProperty(CUSTOM_KEY); Assert.assertEquals("value", prop.getOrDefault().getValue()); + + ConfigurationManager.getConfigInstance().setProperty("testValueOf.ribbon.CustomValueOf", "value2"); + Assert.assertEquals("value2", prop.getOrDefault().getValue()); } @Test @@ -207,5 +230,21 @@ public class ClientConfigTest { Property prop = clientConfig.getScopedProperty(new CommonClientConfigKey("foo.ScopePropertyTimeout", 0) {}); Assert.assertEquals(1000, prop.get().get().intValue()); } + + @Test + public void testDynamicConfig() { + ConfigurationManager.getConfigInstance().setProperty("testValueOf.ribbon.CustomValueOf", "value"); + + DefaultClientConfigImpl clientConfig = new DefaultClientConfigImpl(); + clientConfig.loadProperties("testValueOf"); + + Assert.assertEquals("value", clientConfig.get(CUSTOM_KEY).getValue()); + + ConfigurationManager.getConfigInstance().setProperty("testValueOf.ribbon.CustomValueOf", "value2"); + Assert.assertEquals("value2", clientConfig.get(CUSTOM_KEY).getValue()); + + ConfigurationManager.getConfigInstance().clearProperty("testValueOf.ribbon.CustomValueOf"); + Assert.assertNull(clientConfig.get(CUSTOM_KEY)); + } } diff --git a/ribbon-core/src/test/java/com/netflix/client/config/ReloadableClientConfigTest.java b/ribbon-core/src/test/java/com/netflix/client/config/ReloadableClientConfigTest.java index 54305020335a0ca4d926f941681eec4359307150..2566f42b8f712a517ad524c6ceb66b6a0e7afd35 100644 --- a/ribbon-core/src/test/java/com/netflix/client/config/ReloadableClientConfigTest.java +++ b/ribbon-core/src/test/java/com/netflix/client/config/ReloadableClientConfigTest.java @@ -15,7 +15,7 @@ public class ReloadableClientConfigTest { @Before public void before() { - this.testKey = new CommonClientConfigKey(testName.getMethodName(), -1) {}; + this.testKey = new CommonClientConfigKey(getClass().getName() + "." + testName.getMethodName(), -1) {}; } @Test @@ -32,12 +32,25 @@ public class ReloadableClientConfigTest { @Test public void setBeforeLoading() { + // Ensure property is set before config is created ConfigurationManager.getConfigInstance().setProperty("ribbon." + testKey.key(), "123"); + // Load config and attempt to set value to 0 final DefaultClientConfigImpl config = new DefaultClientConfigImpl(); config.loadProperties("foo"); + config.set(testKey, 0); + // Value should be 123 because of fast property Assert.assertEquals(123, config.get(testKey).intValue()); + + // Clearing property should make it null + ConfigurationManager.getConfigInstance().clearProperty("ribbon." + testKey.key()); + + Assert.assertNull(config.get(testKey)); + + // Setting property again should give new value + ConfigurationManager.getConfigInstance().setProperty("ribbon." + testKey.key(), "124"); + Assert.assertEquals(124, config.get(testKey).intValue()); } @Test