Skip to content

Commit

Permalink
feature: Fix returning Property defaults
Browse files Browse the repository at this point in the history 
The current mechanism for returning property default values is a bit awkward because of how property overrides are applied.  This change addresses that by adding an explicit `getOrDefault` api.
  • Loading branch information
@elandau
elandau committed May 15, 2019
1 parent 360024f commit cb2bee2
Show file tree
Hide file tree
Showing 12 changed files with 65 additions and 49 deletions.
18 changes: 14 additions & 4 deletions ribbon-core/src/main/java/com/netflix/client/config/FallbackProperty.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.netflix.client.config;

import java.util.Optional;
import java.util.function.Consumer;

public final class FallbackProperty<T> implements Property<T> {
Expand All @@ -13,13 +14,22 @@ public FallbackProperty(Property<T> primary, Property<T> fallback) {

@Override
public void onChange(Consumer<T> consumer) {
primary.onChange(ignore -> consumer.accept(get()));
fallback.onChange(ignore -> consumer.accept(get()));
primary.onChange(ignore -> consumer.accept(getOrDefault()));
fallback.onChange(ignore -> consumer.accept(getOrDefault()));
}

@Override
public T get() {
return primary.getOptional().orElseGet(fallback::get);
public Optional<T> get() {
Optional<T> value = primary.get();
if (value.isPresent()) {
return value;
}
return fallback.get();
}

@Override
public T getOrDefault() {
return primary.get().orElseGet(fallback::getOrDefault);
}

@Override
Expand Down
15 changes: 10 additions & 5 deletions ribbon-core/src/main/java/com/netflix/client/config/Property.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@ public interface Property<T> {
void onChange(Consumer<T> consumer);

/**
* @return Get the current value. Can be null if no default value was defined
* @return Get the current value. Can be null if not set
*/
T get();
Optional<T> get();

/**
* @return Return the value only if not set. Will return Optional.empty() instead of default value if not set
* @return Get the current value or the default value if not set
*/
default Optional<T> getOptional() { return Optional.ofNullable(get()); }
T getOrDefault();

default Property<T> fallbackWith(Property<T> fallback) {
return new FallbackProperty<>(this, fallback);
Expand All @@ -36,7 +36,12 @@ public void onChange(Consumer<T> consumer) {
}

@Override
public T get() {
public Optional<T> get() {
return Optional.ofNullable(value);
}

@Override
public T getOrDefault() {
return value;
}

Expand Down
31 changes: 16 additions & 15 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 @@ -119,7 +119,7 @@ public final Map<String, Object> getProperties() {
Map<String, Object> result = new HashMap<>(dynamicProperties.size());

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

LOG.info(result.toString());
Expand All @@ -136,7 +136,7 @@ public void forEach(BiConsumer<IClientConfigKey<?>, Object> consumer) {
dynamicProperties.forEach((key, value) -> consumer.accept(key, value.get()));
}

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

return new ReloadableProperty<T>() {
Expand All @@ -151,24 +151,24 @@ private <T> ReloadableProperty<T> createProperty(final Supplier<Optional<T>> val

@Override
public void onChange(Consumer<T> consumer) {
final AtomicReference<Optional<T>> previous = new AtomicReference<>(getOptional());
final AtomicReference<Optional<T>> previous = new AtomicReference<>(get());
changeActions.add(() -> {
Optional<T> current = getOptional();
Optional<T> current = get();
if (!current.equals(Optional.ofNullable(previous.get()))) {
previous.set(current);
consumer.accept(current.orElseGet(defaultValue));
consumer.accept(current.orElseGet(defaultSupplier::get));
}
});
}

@Override
public T get() {
return value.orElseGet(defaultValue);
public Optional<T> get() {
return value;
}

@Override
public Optional<T> getOptional() {
return value;
public T getOrDefault() {
return value.orElse(defaultSupplier.get());
}

@Override
Expand All @@ -186,7 +186,7 @@ public String toString() {

@Override
public final <T> T get(IClientConfigKey<T> key) {
return (T)Optional.ofNullable(getInternal(key)).flatMap(Property::getOptional).orElse(null);
return getInternal(key).get().orElse(null);
}

public final <T> ReloadableProperty<T> getInternal(IClientConfigKey<T> key) {
Expand Down Expand Up @@ -277,7 +277,8 @@ protected <T> Optional<T> resolveDefaultProperty(IClientConfigKey<T> key) {
.orElseThrow(() -> new IllegalArgumentException("Unsupported value type `" + type + "'"));
}
} else {
throw new IllegalArgumentException("Incompatible value type `" + value.getClass() + "` while expecting '" + type + "`");
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);
Expand Down Expand Up @@ -330,7 +331,7 @@ public Object getProperty(IClientConfigKey key) {
@Override
@Deprecated
public Object getProperty(IClientConfigKey key, Object defaultVal) {
return Optional.ofNullable(getInternal(key).get()).orElse(defaultVal);
return getInternal(key).get().orElse(defaultVal);
}

@Override
Expand Down Expand Up @@ -388,13 +389,13 @@ public long getRefreshCount() {

private String generateToString() {
return "ClientConfig:" + dynamicProperties.entrySet().stream()
.map((t) -> {
.map(t -> {
if (t.getKey().key().endsWith("Password")) {
return t.getKey() + ":***";
}
Object value = t.getValue().get();
Optional value = t.getValue().get();
Object defaultValue = t.getKey().defaultValue();
return t.getKey() + ":" + MoreObjects.firstNonNull(value, defaultValue);
return t.getKey() + ":" + value.orElse(defaultValue);
})
.collect(Collectors.joining(", "));
}
Expand Down
2 changes: 1 addition & 1 deletion ribbon-core/src/main/java/com/netflix/client/config/UnboxedIntProperty.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ public class UnboxedIntProperty {
private volatile int value;

public UnboxedIntProperty(Property<Integer> delegate) {
this.value = delegate.get();
this.value = delegate.getOrDefault();

delegate.onChange(newValue -> this.value = newValue);
}
Expand Down
8 changes: 4 additions & 4 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 @@ -130,7 +130,7 @@ public void testFallback_noneSet() {
Property<Integer> prop = clientConfig.getGlobalProperty(INTEGER_PROPERTY.format(testName.getMethodName()))
.fallbackWith(clientConfig.getGlobalProperty(DEFAULT_INTEGER_PROPERTY));

Assert.assertEquals(30, prop.get().intValue());
Assert.assertEquals(30, prop.getOrDefault().intValue());
}

@Test
Expand All @@ -140,7 +140,7 @@ public void testFallback_fallbackSet() {
Property<Integer> prop = clientConfig.getGlobalProperty(INTEGER_PROPERTY.format(testName.getMethodName()))
.fallbackWith(clientConfig.getGlobalProperty(DEFAULT_INTEGER_PROPERTY));

Assert.assertEquals(100, prop.get().intValue());
Assert.assertEquals(100, prop.getOrDefault().intValue());
}

@Test
Expand All @@ -152,7 +152,7 @@ public void testFallback_primarySet() {
Property<Integer> prop = clientConfig.getGlobalProperty(INTEGER_PROPERTY.format(testName.getMethodName()))
.fallbackWith(clientConfig.getGlobalProperty(DEFAULT_INTEGER_PROPERTY));

Assert.assertEquals(200, prop.get().intValue());
Assert.assertEquals(200, prop.getOrDefault().intValue());
}

static class CustomValueOf {
Expand Down Expand Up @@ -189,7 +189,7 @@ public void testValueOf() {
clientConfig.setClientName("testValueOf");

Property<CustomValueOf> prop = clientConfig.getDynamicProperty(CUSTOM_KEY);
Assert.assertEquals("value", prop.get().getValue());
Assert.assertEquals("value", prop.getOrDefault().getValue());
}
}

2 changes: 1 addition & 1 deletion ribbon-httpclient/src/main/java/com/netflix/http4/ConnectionPoolCleaner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public void run() {

void cleanupConnections(){
connMgr.closeExpiredConnections();
connMgr.closeIdleConnections(connIdleEvictTimeMilliSeconds.get(), TimeUnit.MILLISECONDS);
connMgr.closeIdleConnections(connIdleEvictTimeMilliSeconds.getOrDefault(), TimeUnit.MILLISECONDS);
}

public void shutdown() {
Expand Down
8 changes: 4 additions & 4 deletions ribbon-httpclient/src/main/java/com/netflix/http4/NFHttpClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,8 @@ void init(IClientConfig config, boolean registerMonitor) {

this.sleepTimeFactorMsProperty = config.getGlobalProperty(SLEEP_TIME_FACTOR_MS.format(name));
setHttpRequestRetryHandler(
new NFHttpMethodRetryHandler(this.name, this.retriesProperty.get(), false,
this.sleepTimeFactorMsProperty.get()));
new NFHttpMethodRetryHandler(this.name, this.retriesProperty.getOrDefault(), false,
this.sleepTimeFactorMsProperty.getOrDefault()));
tracer = Monitors.newTimer(EXECUTE_TRACER + "-" + name, TimeUnit.MILLISECONDS);
if (registerMonitor) {
Monitors.registerObject(name, this);
Expand Down Expand Up @@ -233,7 +233,7 @@ public int getMaxConnectionsPerHost() {

@Monitor(name = "HttpClient-NumRetries", type = DataSourceType.INFORMATIONAL)
public int getNumRetries() {
return this.retriesProperty.get();
return this.retriesProperty.getOrDefault();
}

public void setConnIdleEvictTimeMilliSeconds(Property<Integer> connIdleEvictTimeMilliSeconds) {
Expand All @@ -242,7 +242,7 @@ public void setConnIdleEvictTimeMilliSeconds(Property<Integer> connIdleEvictTime

@Monitor(name = "HttpClient-SleepTimeFactorMs", type = DataSourceType.INFORMATIONAL)
public int getSleepTimeFactorMs() {
return this.sleepTimeFactorMsProperty.get();
return this.sleepTimeFactorMsProperty.getOrDefault();
}

// copied from httpclient source code
Expand Down
6 changes: 3 additions & 3 deletions ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/AvailabilityPredicate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ private void initDynamicProperty(IClientConfig clientConfig) {
}

private int getActiveConnectionsLimit() {
Integer limit = activeConnectionsLimit.get();
Integer limit = activeConnectionsLimit.getOrDefault();
if (limit == -1) {
limit = defaultActiveConnectionsLimit.get();
limit = defaultActiveConnectionsLimit.getOrDefault();
if (limit == -1) {
limit = Integer.MAX_VALUE;
}
Expand All @@ -90,7 +90,7 @@ public boolean apply(@Nullable PredicateKey input) {
}

private boolean shouldSkipServer(ServerStats stats) {
if ((circuitBreakerFiltering.get() && stats.isCircuitBreakerTripped())
if ((circuitBreakerFiltering.getOrDefault() && stats.isCircuitBreakerTripped())
|| stats.getActiveRequestsCount() >= getActiveConnectionsLimit()) {
return true;
}
Expand Down
8 changes: 4 additions & 4 deletions ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/ServerListSubsetFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,17 +111,17 @@ public List<T> getFilteredListOfServers(List<T> servers) {
} else {
ServerStats stats = lbStats.getSingleServerStat(server);
// remove the servers that do not meet health criteria
if (stats.getActiveRequestsCount() > eliminationConnectionCountThreshold.get()
|| stats.getFailureCount() > eliminationFailureCountThreshold.get()) {
if (stats.getActiveRequestsCount() > eliminationConnectionCountThreshold.getOrDefault()
|| stats.getFailureCount() > eliminationFailureCountThreshold.getOrDefault()) {
newSubSet.remove(server);
// also remove from the general pool to avoid selecting them again
candidates.remove(server);
}
}
}
int targetedListSize = sizeProp.get();
int targetedListSize = sizeProp.getOrDefault();
int numEliminated = currentSubset.size() - newSubSet.size();
int minElimination = (int) (targetedListSize * eliminationPercent.get());
int minElimination = (int) (targetedListSize * eliminationPercent.getOrDefault());
int numToForceEliminate = 0;
if (targetedListSize < newSubSet.size()) {
// size is shrinking
Expand Down
8 changes: 4 additions & 4 deletions ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/ZoneAffinityServerListFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public ZoneAffinityServerListFilter(IClientConfig niwsClientConfig) {
public void initWithNiwsConfig(IClientConfig niwsClientConfig) {
zoneAffinity = niwsClientConfig.getOrDefault(CommonClientConfigKey.EnableZoneAffinity);
zoneExclusive = niwsClientConfig.getOrDefault(CommonClientConfigKey.EnableZoneExclusivity);
zone = niwsClientConfig.getGlobalProperty(ZONE).get();
zone = niwsClientConfig.getGlobalProperty(ZONE).getOrDefault();
zoneAffinityPredicate = new ZoneAffinityPredicate(zone);

activeReqeustsPerServerThreshold = niwsClientConfig.getDynamicProperty(MAX_LOAD_PER_SERVER);
Expand Down Expand Up @@ -107,9 +107,9 @@ private boolean shouldEnableZoneAffinity(List<T> filtered) {
double loadPerServer = snapshot.getLoadPerServer();
int instanceCount = snapshot.getInstanceCount();
int circuitBreakerTrippedCount = snapshot.getCircuitTrippedCount();
if (((double) circuitBreakerTrippedCount) / instanceCount >= blackOutServerPercentageThreshold.get()
|| loadPerServer >= activeReqeustsPerServerThreshold.get()
|| (instanceCount - circuitBreakerTrippedCount) < availableServersThreshold.get()) {
if (((double) circuitBreakerTrippedCount) / instanceCount >= blackOutServerPercentageThreshold.getOrDefault()
|| loadPerServer >= activeReqeustsPerServerThreshold.getOrDefault()
|| (instanceCount - circuitBreakerTrippedCount) < availableServersThreshold.getOrDefault()) {
logger.debug("zoneAffinity is overriden. blackOutServerPercentage: {}, activeReqeustsPerServer: {}, availableServers: {}",
new Object[] {(double) circuitBreakerTrippedCount / instanceCount, loadPerServer, instanceCount - circuitBreakerTrippedCount});
return false;
Expand Down
4 changes: 2 additions & 2 deletions ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/ZoneAvoidancePredicate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ private void initDynamicProperties(IClientConfig clientConfig) {

@Override
public boolean apply(@Nullable PredicateKey input) {
if (!enabled.get()) {
if (!enabled.getOrDefault()) {
return true;
}
String serverZone = input.getServer().getZone();
Expand All @@ -102,7 +102,7 @@ public boolean apply(@Nullable PredicateKey input) {
return true;
}
logger.debug("Zone snapshots: {}", zoneSnapshot);
Set<String> availableZones = ZoneAvoidanceRule.getAvailableZones(zoneSnapshot, triggeringLoad.get(), triggeringBlackoutPercentage.get());
Set<String> availableZones = ZoneAvoidanceRule.getAvailableZones(zoneSnapshot, triggeringLoad.getOrDefault(), triggeringBlackoutPercentage.getOrDefault());
logger.debug("Available zones: {}", availableZones);
if (availableZones != null) {
return availableZones.contains(input.getServer().getZone());
Expand Down
4 changes: 2 additions & 2 deletions ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/ZoneAwareLoadBalancer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ protected void setServerListForZones(Map<String, List<Server>> zoneServersMap) {

@Override
public Server chooseServer(Object key) {
if (!enabled.get() || getLoadBalancerStats().getAvailableZones().size() <= 1) {
if (!enabled.getOrDefault() || getLoadBalancerStats().getAvailableZones().size() <= 1) {
logger.debug("Zone aware logic disabled or there is only one zone");
return super.chooseServer(key);
}
Expand All @@ -148,7 +148,7 @@ public Server chooseServer(Object key) {
LoadBalancerStats lbStats = getLoadBalancerStats();
Map<String, ZoneSnapshot> zoneSnapshot = ZoneAvoidanceRule.createSnapshot(lbStats);
logger.debug("Zone snapshots: {}", zoneSnapshot);
Set<String> availableZones = ZoneAvoidanceRule.getAvailableZones(zoneSnapshot, triggeringLoad.get(), triggeringBlackoutPercentage.get());
Set<String> availableZones = ZoneAvoidanceRule.getAvailableZones(zoneSnapshot, triggeringLoad.getOrDefault(), triggeringBlackoutPercentage.getOrDefault());
logger.debug("Available zones: {}", availableZones);
if (availableZones != null && availableZones.size() < zoneSnapshot.keySet().size()) {
String zone = ZoneAvoidanceRule.randomChooseZone(zoneSnapshot, availableZones);
Expand Down

0 comments on commit cb2bee2

Please sign in to comment.