Skip to content

Commit

Permalink
Merge pull request #419 from elandau/bugfix/change_notification
Browse files Browse the repository at this point in the history 
config: Fix excessive change notifications
  • Loading branch information
@elandau
elandau authored May 19, 2019
2 parents 2a13e39 + 3849d85 commit 532285a
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 44 deletions.
32 changes: 24 additions & 8 deletions 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 @@ -9,6 +9,7 @@

import java.util.Arrays;
import java.util.Optional;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.TimeUnit;
import java.util.function.BiConsumer;
import java.util.stream.Collectors;
Expand All @@ -18,9 +19,27 @@ public class ArchaiusPropertyResolver implements PropertyResolver {

public static final ArchaiusPropertyResolver INSTANCE = new ArchaiusPropertyResolver();
private final AbstractConfiguration config;
private final CopyOnWriteArrayList<Runnable> actions = new CopyOnWriteArrayList<>();

private ArchaiusPropertyResolver() {
this.config = ConfigurationManager.getConfigInstance();

ConfigurationManager.getConfigInstance().addConfigurationListener(new ConfigurationListener() {
@Override
public void configurationChanged(ConfigurationEvent event) {
if (!event.isBeforeUpdate()) {
actions.forEach(ArchaiusPropertyResolver::invokeAction);
}
}
});
}

private static void invokeAction(Runnable action) {
try {
action.run();
} catch (Exception e) {
LOG.info("Failed to invoke action", e);
}
}

@Override
Expand Down Expand Up @@ -66,13 +85,10 @@ public void forEach(String prefix, BiConsumer<String, String> consumer) {

@Override
public void onChange(Runnable action) {
ConfigurationManager.getConfigInstance().addConfigurationListener(new ConfigurationListener() {
@Override
public void configurationChanged(ConfigurationEvent event) {
if (!event.isBeforeUpdate()) {
action.run();
}
}
});
actions.add(action);
}

public int getActionCount() {
return actions.size();
}
}
77 changes: 41 additions & 36 deletions ribbon-core/src/main/java/com/netflix/client/config/ReloadableClientConfig.java
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
package com.netflix.client.config;

import com.google.common.base.MoreObjects;
import com.google.common.base.Preconditions;
import org.apache.commons.lang.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.lang.reflect.InvocationTargetException;
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 {
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> defaultProperties = new HashMap<>();
private final Map<String, 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().
Expand All @@ -60,17 +59,18 @@ public abstract class ReloadableClientConfig implements IClientConfig {

private boolean isDynamic;

private boolean isLoaded = false;

protected ReloadableClientConfig(PropertyResolver resolver) {
this(resolver, DEFAULT_CLIENT_NAME);
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;

resolver.onChange(this::reload);
}

protected PropertyResolver getPropertyResolver() {
Expand All @@ -85,6 +85,10 @@ public final void reload() {
cachedToString = null;
}

/**
* @deprecated Use {@link #loadProperties(String)}
*/
@Deprecated
public void setClientName(String clientName){
this.clientName = clientName;
}
Expand All @@ -106,6 +110,12 @@ public final void setNameSpace(String nameSpace) {

@Override
public void loadProperties(String clientName) {
Preconditions.checkState(isLoaded == false, "Config '{}' can only be loaded once", clientName);
if (!isLoaded) {
this.isLoaded = true;
resolver.onChange(this::reload);
}

this.isDynamic = true;
this.clientName = clientName;
loadDefaultValues();
Expand All @@ -119,27 +129,20 @@ public void loadDefaultValues() {

@Override
public final Map<String, Object> getProperties() {
Map<String, Object> result = new HashMap<>(dynamicProperties.size());

dynamicProperties.forEach((key, prop) ->
prop.get().ifPresent(value -> result.put(key.key(), String.valueOf(value)))
);

LOG.info(result.toString());
return result;
return Collections.unmodifiableMap(internalProperties);
}

@Override
public void forEachDefault(BiConsumer<IClientConfigKey<?>, Object> consumer) {
dynamicProperties.forEach((key, value) -> consumer.accept(key, defaultProperties.get(key.key())));
dynamicProperties.forEach((key, value) -> consumer.accept(key, internalProperties.get(key.key())));
}

@Override
public void forEach(BiConsumer<IClientConfigKey<?>, Object> consumer) {
dynamicProperties.forEach((key, value) -> consumer.accept(key, value.get().orElse(null)));
}

private <T> ReloadableProperty<T> createProperty(final Supplier<Optional<T>> valueSupplier, final Supplier<T> defaultSupplier, final boolean isDynamic) {
private <T> ReloadableProperty<T> createProperty(final Supplier<Optional<T>> valueSupplier, final Supplier<T> defaultSupplier) {
Preconditions.checkNotNull(valueSupplier, "defaultValueSupplier cannot be null");

return new ReloadableProperty<T>() {
Expand Down Expand Up @@ -189,11 +192,15 @@ public String toString() {

@Override
public final <T> T get(IClientConfigKey<T> key) {
return getInternal(key).get().orElse(null);
if (isLoaded) {
return getOrCreateProperty(key).get().orElse(null);
} else {
return getIfSet(key).orElse(null);
}
}

public final <T> ReloadableProperty<T> getInternal(IClientConfigKey<T> key) {
return (ReloadableProperty<T>)dynamicProperties.computeIfAbsent(key, ignore -> getClientDynamicProperty(key, isDynamic));
private final <T> ReloadableProperty<T> getOrCreateProperty(IClientConfigKey<T> key) {
return (ReloadableProperty<T>) dynamicProperties.computeIfAbsent(key, ignore -> getClientDynamicProperty(key));
}

@Override
Expand All @@ -202,21 +209,19 @@ public final <T> Property<T> getGlobalProperty(IClientConfigKey<T> key) {

return (Property<T>) dynamicProperties.computeIfAbsent(key, ignore -> createProperty(
() -> resolver.get(key.key(), key.type()),
key::defaultValue,
true));
key::defaultValue));
}

interface ReloadableProperty<T> extends Property<T> {
void refresh();
}

private <T> ReloadableProperty<T> getClientDynamicProperty(IClientConfigKey<T> key, boolean isDynamic) {
private <T> ReloadableProperty<T> getClientDynamicProperty(IClientConfigKey<T> key) {
LOG.debug("Get dynamic property key={} ns={} client={}", key.key(), getNameSpace(), clientName);

return createProperty(
() -> resolveFinalProperty(key),
key::defaultValue,
isDynamic);
key::defaultValue);
}

/**
Expand Down Expand Up @@ -257,7 +262,7 @@ private <T> Optional<T> resolverScopedProperty(IClientConfigKey<T> key) {

@Override
public <T> Optional<T> getIfSet(IClientConfigKey<T> key) {
return Optional.ofNullable(defaultProperties.get(key.key()))
return Optional.ofNullable(internalProperties.get(key.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
Expand Down Expand Up @@ -299,23 +304,21 @@ public <T> Optional<T> getIfSet(IClientConfigKey<T> key) {

@Override
public final <T> Property<T> getDynamicProperty(IClientConfigKey<T> key) {
return getClientDynamicProperty(key, true);
return getClientDynamicProperty(key);
}

@Override
public <T> Property<T> getScopedProperty(IClientConfigKey<T> key) {
return (Property<T>) dynamicProperties.computeIfAbsent(key, ignore -> createProperty(
() -> resolverScopedProperty(key),
key::defaultValue,
isDynamic));
key::defaultValue));
}

@Override
public <T> Property<T> getPrefixMappedProperty(IClientConfigKey<T> key) {
return (Property<T>) dynamicProperties.computeIfAbsent(key, ignore -> createProperty(
getPrefixedMapPropertySupplier(key),
key::defaultValue,
isDynamic));
key::defaultValue));
}

private <T> Supplier<Optional<T>> getPrefixedMapPropertySupplier(IClientConfigKey<T> key) {
Expand Down Expand Up @@ -354,12 +357,14 @@ public final <T> IClientConfig set(IClientConfigKey<T> key, T value) {
Preconditions.checkArgument(key != null, "key cannot be null");
// Treat nulls as deletes
if (value == null) {
defaultProperties.remove(key.key());
internalProperties.remove(key.key());
} else {
defaultProperties.put(key.key(), value);
internalProperties.put(key.key(), value);
}

getInternal(key).refresh();
if (isLoaded) {
getOrCreateProperty(key).refresh();
}
cachedToString = null;

return this;
Expand All @@ -375,13 +380,13 @@ public void setProperty(IClientConfigKey key, Object value) {
@Override
@Deprecated
public Object getProperty(IClientConfigKey key) {
return getInternal(key).get().orElse(null);
return get(key);
}

@Override
@Deprecated
public Object getProperty(IClientConfigKey key, Object defaultVal) {
return getInternal(key).get().orElse(defaultVal);
return getOrCreateProperty(key).get().orElse(defaultVal);
}

@Override
Expand Down Expand Up @@ -413,7 +418,7 @@ public IClientConfig applyOverride(IClientConfig override) {
return this;
}

this.defaultProperties.putAll(override.getProperties());
this.internalProperties.putAll(override.getProperties());
reload();
return this;
}
Expand Down
2 changes: 2 additions & 0 deletions ribbon-core/src/test/resources/log4j.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@ log4j.rootCategory=INFO,stdout
log4j.appender.stdout=org.apache.log4j.ConsoleAppender
log4j.appender.stdout.layout=org.apache.log4j.PatternLayout
log4j.appender.stdout.layout.ConversionPattern=%d %-5p %C:%L [%t] [%M] %m%n

log4j.logger.com.netflix.client.config=DEBUG

0 comments on commit 532285a

Please sign in to comment.