Skip to content

Commit

Permalink
Merge pull request #427 from elandau/bugfix/reduce_property_resolution
Browse files Browse the repository at this point in the history 
config: Fix property resolution when config not loaded
  • Loading branch information
@elandau
elandau authored May 23, 2019
2 parents 782fb40 + 7c3e716 commit 15cfb49
Show file tree
Hide file tree
Showing 8 changed files with 87 additions and 147 deletions.
2 changes: 1 addition & 1 deletion ribbon-archaius/src/main/java/com/netflix/client/config/ArchaiusPropertyResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public <T> Optional<T> get(String key, Class<T> type) {
if (type.equals(String.class)) {
return (T)value;
} else {
return PropertyResolver.resolveWithValueOf(type, value)
return PropertyUtils.resolveWithValueOf(type, value)
.orElseThrow(() -> new IllegalArgumentException("Unable to convert value to desired type " + type));
}
});
Expand Down
4 changes: 0 additions & 4 deletions ribbon-core/src/main/java/com/netflix/client/config/AbstractDefaultClientConfigImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,6 @@ protected AbstractDefaultClientConfigImpl(PropertyResolver resolver) {
super(resolver);
}

protected AbstractDefaultClientConfigImpl(PropertyResolver resolver, String clientName) {
super(resolver, clientName);
}

public void setVipAddressResolver(VipAddressResolver resolver) {
this.vipResolver = resolver;
}
Expand Down
7 changes: 0 additions & 7 deletions ribbon-core/src/main/java/com/netflix/client/config/IClientConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,13 +147,6 @@ default <T> Optional<T> getIfSet(IClientConfigKey<T> key) {
*/
<T> Property<T> getDynamicProperty(IClientConfigKey<T> key);

/**
* @return Get a property that is only scoped to this client.
*/
default <T> Property<T> getScopedProperty(IClientConfigKey<T> key) {
throw new UnsupportedOperationException();
}

/**
* @return Return a dynamically updated property that is a mapping of all properties prefixed by the key name to an
* object with static method valueOf(Map{@literal <}String, String{@literal >})
Expand Down
33 changes: 0 additions & 33 deletions ribbon-core/src/main/java/com/netflix/client/config/PropertyResolver.java
View file
Original file line number Diff line number Diff line change
@@ -1,20 +1,12 @@
package com.netflix.client.config;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.lang.reflect.Method;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.BiConsumer;

/**
* Internal abstraction to decouple the property source from Ribbon's internal configuration.
*/
public interface PropertyResolver {
Logger LOG = LoggerFactory.getLogger(PropertyResolver.class);

/**
* @return Get the value of a property or Optional.empty() if not set
*/
Expand All @@ -30,29 +22,4 @@ public interface PropertyResolver {
* @param action
*/
void onChange(Runnable action);

/**
* Returns the internal property to the desiredn type
*/
Map<Class<?>, Optional<Method>> valueOfMethods = new ConcurrentHashMap<>();

static <T> Optional<T> resolveWithValueOf(Class<T> type, String value) {
return valueOfMethods.computeIfAbsent(type, ignore -> {
try {
return Optional.of(type.getDeclaredMethod("valueOf", String.class));
} catch (NoSuchMethodException e) {
return Optional.empty();
} catch (Exception e) {
LOG.warn("Unable to determine if type " + type + " has a valueOf() static method", e);
return Optional.empty();
}
}).map(method -> {
try {
return (T)method.invoke(null, value);
} catch (Exception e) {
throw new RuntimeException(e);
}
});
}

}
40 changes: 40 additions & 0 deletions ribbon-core/src/main/java/com/netflix/client/config/PropertyUtils.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package com.netflix.client.config;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.lang.reflect.Method;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;

public class PropertyUtils {
private static Logger LOG = LoggerFactory.getLogger(PropertyUtils.class);

private PropertyUtils() {}

/**
* Returns the internal property to the desiredn type
*/
private static Map<Class<?>, Optional<Method>> valueOfMethods = new ConcurrentHashMap<>();

public static <T> Optional<T> resolveWithValueOf(Class<T> type, String value) {
return valueOfMethods.computeIfAbsent(type, ignore -> {
try {
return Optional.of(type.getDeclaredMethod("valueOf", String.class));
} catch (NoSuchMethodException e) {
return Optional.empty();
} catch (Exception e) {
LOG.warn("Unable to determine if type " + type + " has a valueOf() static method", e);
return Optional.empty();
}
}).map(method -> {
try {
return (T)method.invoke(null, value);
} catch (Exception e) {
throw new RuntimeException(e);
}
});
}

}
130 changes: 43 additions & 87 deletions ribbon-core/src/main/java/com/netflix/client/config/ReloadableClientConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import org.slf4j.LoggerFactory;

import java.lang.reflect.Method;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -40,7 +39,7 @@ public abstract class ReloadableClientConfig implements IClientConfig {

// 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<IClientConfigKey, Optional<Object>> internalProperties = new ConcurrentHashMap<>();
private final Map<IClientConfigKey, Optional<?>> internalProperties = new ConcurrentHashMap<>();

private final Map<IClientConfigKey, ReloadableProperty<?>> dynamicProperties = new ConcurrentHashMap<>();

Expand All @@ -52,31 +51,13 @@ public abstract class ReloadableClientConfig implements IClientConfig {

private final PropertyResolver resolver;

private String clientName;
private String clientName = DEFAULT_CLIENT_NAME;

private String namespace = DEFAULT_NAMESPACE;

private boolean isLoaded = false;

/**
* @deprecated Use {@link #ReloadableClientConfig(PropertyResolver, String, String)}
*/
@Deprecated
protected ReloadableClientConfig(PropertyResolver resolver) {
this(resolver, DEFAULT_CLIENT_NAME);
}

/**
* @deprecated Use {@link #ReloadableClientConfig(PropertyResolver, String, String)}
*/
@Deprecated
protected ReloadableClientConfig(PropertyResolver resolver, String clientName) {
this(resolver, DEFAULT_NAMESPACE, DEFAULT_CLIENT_NAME);
}

protected ReloadableClientConfig(PropertyResolver resolver, String namespace, String clientName) {
this.namespace = namespace;
this.clientName = clientName;
this.resolver = resolver;
}

Expand Down Expand Up @@ -119,10 +100,14 @@ public final void setNameSpace(String nameSpace) {
@Override
public void loadProperties(String clientName) {
Preconditions.checkState(!isLoaded, "Config '{}' can only be loaded once", clientName);

LOG.info("Loading config for `{}`", clientName);
this.clientName = clientName;
this.isLoaded = true;
loadDefaultValues();
resolver.onChange(this::reload);

internalProperties.forEach((key, value) -> LOG.info("{} : {} -> {}", clientName, key, value.orElse(null)));
}

/**
Expand All @@ -146,27 +131,29 @@ public void forEach(BiConsumer<IClientConfigKey<?>, Object> consumer) {
}

/**
* Create an action that will refresh the cached value for key. Uses the current value as a reference and will
* Register 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 <T> Runnable createAutoRefreshAction(final IClientConfigKey<T> key, final Supplier<Optional<T>> valueSupplier) {
final AtomicReference<Optional<T>> current = new AtomicReference<>(valueSupplier.get());
return () -> {
final Optional<T> 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<Object>) next);
} else {
internalProperties.put(key, Optional.empty());
}
private <T> void autoRefreshFromPropertyResolver(final IClientConfigKey<T> key) {
changeActions.computeIfAbsent(key, ignore -> {
final Supplier<Optional<T>> valueSupplier = () -> resolveFromPropertyResolver(key);
final Optional<T> current = valueSupplier.get();
if (current.isPresent()) {
internalProperties.put(key, current);
}
};

final AtomicReference<Optional<T>> previous = new AtomicReference<>(current);
return () -> {
final Optional<T> next = valueSupplier.get();
if (!next.equals(previous.get())) {
LOG.info("New value {}: {} -> {}", key.key(), previous.get(), next);
previous.set(next);
internalProperties.put(key, next);
}
};
});
}

interface ReloadableProperty<T> extends Property<T> {
Expand Down Expand Up @@ -221,8 +208,12 @@ public String toString() {
public final <T> T get(IClientConfigKey<T> key) {
Optional<T> value = (Optional<T>)internalProperties.get(key);
if (value == null) {
set(key, null);
value = (Optional<T>)internalProperties.get(key);
if (!isLoaded) {
return null;
} else {
set(key, null);
value = (Optional<T>) internalProperties.get(key);
}
}

return value.orElse(null);
Expand All @@ -242,21 +233,13 @@ public final <T> Property<T> getGlobalProperty(IClientConfigKey<T> key) {
public final <T> Property<T> getDynamicProperty(IClientConfigKey<T> key) {
LOG.debug("Get dynamic property key={} ns={} client={}", key.key(), getNameSpace(), clientName);

get(key);

return getOrCreateProperty(
key,
() -> (Optional<T>)internalProperties.get(key),
key::defaultValue);
}

@Override
public <T> Property<T> getScopedProperty(IClientConfigKey<T> key) {
LOG.debug("Get dynamic property key={} ns={} client={}", key.key(), getNameSpace(), clientName);
if (isLoaded) {
autoRefreshFromPropertyResolver(key);
}

return getOrCreateProperty(
key,
() -> resolverScopedProperty(key),
() -> (Optional<T>)internalProperties.getOrDefault(key, Optional.empty()),
key::defaultValue);
}

Expand All @@ -280,7 +263,7 @@ public <T> Property<T> getPrefixMappedProperty(IClientConfigKey<T> key) {
* @param <T>
* @return
*/
private <T> Optional<T> resolveFinalProperty(IClientConfigKey<T> key) {
private <T> Optional<T> resolveFromPropertyResolver(IClientConfigKey<T> key) {
Optional<T> value;
if (!StringUtils.isEmpty(clientName)) {
value = resolver.get(clientName + "." + getNameSpace() + "." + key.key(), key.type());
Expand All @@ -292,28 +275,9 @@ private <T> Optional<T> resolveFinalProperty(IClientConfigKey<T> key) {
return resolver.get(getNameSpace() + "." + key.key(), key.type());
}

/**
* 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()) {
return value;
}

return getIfSet(key);
}

@Override
public <T> Optional<T> getIfSet(IClientConfigKey<T> key) {
Optional<T> value = (Optional<T>)internalProperties.get(key);
if (value == null) {
return Optional.empty();
}
return value;
return (Optional<T>)internalProperties.getOrDefault(key, Optional.empty());
}

private <T> T resolveValueToType(IClientConfigKey<T> key, Object value) {
Expand Down Expand Up @@ -343,11 +307,11 @@ private <T> T resolveValueToType(IClientConfigKey<T> key, Object value) {
} else if (TimeUnit.class.equals(type)) {
return (T) TimeUnit.valueOf(strValue);
} else {
return PropertyResolver.resolveWithValueOf(type, strValue)
return PropertyUtils.resolveWithValueOf(type, strValue)
.orElseThrow(() -> new IllegalArgumentException("Unsupported value type `" + type + "'"));
}
} else {
return PropertyResolver.resolveWithValueOf(type, value.toString())
return PropertyUtils.resolveWithValueOf(type, value.toString())
.orElseThrow(() -> new IllegalArgumentException("Incompatible value type `" + value.getClass() + "` while expecting '" + type + "`"));
}
} catch (Exception e) {
Expand Down Expand Up @@ -390,23 +354,15 @@ public final <T> T get(IClientConfigKey<T> key, T defaultValue) {
}

@Override
public final <T> IClientConfig set(IClientConfigKey<T> key, T value) {
public <T> IClientConfig set(IClientConfigKey<T> key, T value) {
Preconditions.checkArgument(key != null, "key cannot be null");

// Make sure the value is property typed.
value = resolveValueToType(key, value);

// Check if there's a dynamic config override available
value = resolveFinalProperty(key).orElse(value);

// Cache the new value
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) {
changeActions.computeIfAbsent(key, ignore -> createAutoRefreshAction(key, () -> resolveFinalProperty(key)))
.run();
internalProperties.put(key, Optional.ofNullable(resolveFromPropertyResolver(key).orElse(value)));
autoRefreshFromPropertyResolver(key);
} else {
internalProperties.put(key, Optional.ofNullable(value));
}
cachedToString = null;

Expand Down
12 changes: 0 additions & 12 deletions ribbon-core/src/test/java/com/netflix/client/config/ClientConfigTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,18 +219,6 @@ public void testValueOf() {
Assert.assertEquals("value2", prop.getOrDefault().getValue());
}

@Test
public void testScopedProperty() {
ConfigurationManager.getConfigInstance().setProperty("ribbon.foo.ScopePropertyTimeout", "2000");
ConfigurationManager.getConfigInstance().setProperty("testScopedProperty.ribbon.foo.ScopePropertyTimeout", "1000");

DefaultClientConfigImpl clientConfig = new DefaultClientConfigImpl();
clientConfig.loadProperties("testScopedProperty");

Property<Integer> prop = clientConfig.getScopedProperty(new CommonClientConfigKey<Integer>("foo.ScopePropertyTimeout", 0) {});
Assert.assertEquals(1000, prop.get().get().intValue());
}

@Test
public void testDynamicConfig() {
ConfigurationManager.getConfigInstance().setProperty("testValueOf.ribbon.CustomValueOf", "value");
Expand Down
6 changes: 3 additions & 3 deletions ribbon-core/src/test/java/com/netflix/client/config/DefaultClientConfigImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ protected NewConfigKey(String configKey) {

@Test
public void testTypedValue() {
ConfigurationManager.getConfigInstance().setProperty("myclient.ribbon." + CommonClientConfigKey.ConnectTimeout, "1000");
ConfigurationManager.getConfigInstance().setProperty("myclient.ribbon." + CommonClientConfigKey.ConnectTimeout, "1500");
DefaultClientConfigImpl config = new DefaultClientConfigImpl();
config.loadProperties("myclient");
assertEquals(1000, config.get(CommonClientConfigKey.ConnectTimeout).intValue());
assertEquals(1500, config.get(CommonClientConfigKey.ConnectTimeout).intValue());
config.set(CommonClientConfigKey.ConnectTimeout, 2000);
// The archaius property should override code override
assertEquals(1000, config.get(CommonClientConfigKey.ConnectTimeout).intValue());
assertEquals(1500, config.get(CommonClientConfigKey.ConnectTimeout).intValue());
}

@Test
Expand Down

0 comments on commit 15cfb49

Please sign in to comment.