From 503995ce5772af720844cedc5acf1e896755311c Mon Sep 17 00:00:00 2001 From: Josep Burgaya Date: Fri, 4 Dec 2020 16:55:58 +0100 Subject: [PATCH] **Motivation** TECHNICAL DEBT: a cyclic dependency between LoadBalancerStats and ServerStats has created a persistent negative impact on developers when working in these classes. Specifically, these classes are very tightly coupled by their shared dynamic configuration attributes. **Changes** REFACTORING: Introduce a new ConfigStats class that encapsulates the configuration attributes shared by LoadBalancerStats and its ServerStats as well as its associated methods. ConfigStats implements the IConfigAware interface. No breaking changes. --- .../com/netflix/loadbalancer/ConfigStats.java | 74 +++++++++++++++++++ .../loadbalancer/LoadBalancerStats.java | 69 +---------------- .../com/netflix/loadbalancer/ServerStats.java | 31 +++----- 3 files changed, 87 insertions(+), 87 deletions(-) create mode 100644 ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/ConfigStats.java diff --git a/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/ConfigStats.java b/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/ConfigStats.java new file mode 100644 index 00000000..3993e4e7 --- /dev/null +++ b/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/ConfigStats.java @@ -0,0 +1,74 @@ +package com.netflix.loadbalancer; + +import com.google.common.base.Preconditions; +import com.netflix.client.config.IClientConfig; +import com.netflix.client.config.IClientConfigKey; +import com.netflix.client.config.CommonClientConfigKey; +import com.netflix.client.config.UnboxedIntProperty; +import com.netflix.client.IClientConfigAware; + +public class ConfigStats implements IClientConfigAware{ + public static final IClientConfigKey ACTIVE_REQUESTS_COUNT_TIMEOUT = new CommonClientConfigKey( + "niws.loadbalancer.serverStats.activeRequestsCount.effectiveWindowSeconds", 60 * 10) {}; + + public static final IClientConfigKey CONNECTION_FAILURE_COUNT_THRESHOLD = new CommonClientConfigKey( + "niws.loadbalancer.%s.connectionFailureCountThreshold", 3) {}; + + public static final IClientConfigKey CIRCUIT_TRIP_TIMEOUT_FACTOR_SECONDS = new CommonClientConfigKey( + "niws.loadbalancer.%s.circuitTripTimeoutFactorSeconds", 10) {}; + + public static final IClientConfigKey CIRCUIT_TRIP_MAX_TIMEOUT_SECONDS = new CommonClientConfigKey( + "niws.loadbalancer.%s.circuitTripMaxTimeoutSeconds", 30) {}; + + public static final IClientConfigKey DEFAULT_CONNECTION_FAILURE_COUNT_THRESHOLD = new CommonClientConfigKey( + "niws.loadbalancer.default.connectionFailureCountThreshold", 3) {}; + + public static final IClientConfigKey DEFAULT_CIRCUIT_TRIP_TIMEOUT_FACTOR_SECONDS = new CommonClientConfigKey( + "niws.loadbalancer.default.circuitTripTimeoutFactorSeconds", 10) {}; + + public static final IClientConfigKey DEFAULT_CIRCUIT_TRIP_MAX_TIMEOUT_SECONDS = new CommonClientConfigKey( + "niws.loadbalancer.default.circuitTripMaxTimeoutSeconds", 30) {}; + + private UnboxedIntProperty connectionFailureThreshold = new UnboxedIntProperty(CONNECTION_FAILURE_COUNT_THRESHOLD.defaultValue()); + + private UnboxedIntProperty circuitTrippedTimeoutFactor = new UnboxedIntProperty(CIRCUIT_TRIP_TIMEOUT_FACTOR_SECONDS.defaultValue()); + + private UnboxedIntProperty maxCircuitTrippedTimeout = new UnboxedIntProperty(CIRCUIT_TRIP_MAX_TIMEOUT_SECONDS.defaultValue()); + + private UnboxedIntProperty activeRequestsCountTimeout = new UnboxedIntProperty(ACTIVE_REQUESTS_COUNT_TIMEOUT.defaultValue()); + + @Override + public void initWithNiwsConfig(IClientConfig clientConfig) { + String name = clientConfig.getClientName(); + Preconditions.checkArgument(name != null, "IClientConfig#getClientName() must not be null"); + this.connectionFailureThreshold = new UnboxedIntProperty( + clientConfig.getGlobalProperty(CONNECTION_FAILURE_COUNT_THRESHOLD.format(name)) + .fallbackWith(clientConfig.getGlobalProperty(DEFAULT_CONNECTION_FAILURE_COUNT_THRESHOLD)) + ); + this.circuitTrippedTimeoutFactor = new UnboxedIntProperty( + clientConfig.getGlobalProperty(CIRCUIT_TRIP_TIMEOUT_FACTOR_SECONDS.format(name)) + .fallbackWith(clientConfig.getGlobalProperty(DEFAULT_CIRCUIT_TRIP_TIMEOUT_FACTOR_SECONDS)) + ); + this.maxCircuitTrippedTimeout = new UnboxedIntProperty( + clientConfig.getGlobalProperty(CIRCUIT_TRIP_MAX_TIMEOUT_SECONDS.format(name)) + .fallbackWith(clientConfig.getGlobalProperty(DEFAULT_CIRCUIT_TRIP_MAX_TIMEOUT_SECONDS)) + ); + this.activeRequestsCountTimeout = new UnboxedIntProperty( + clientConfig.getGlobalProperty(ACTIVE_REQUESTS_COUNT_TIMEOUT)); + } + + int getConnectionFailureThreshold() { + return connectionFailureThreshold.get(); + } + + int getCircuitTrippedTimeoutFactor() { + return circuitTrippedTimeoutFactor.get(); + } + + int getMaxCircuitTrippedTimeout() { + return maxCircuitTrippedTimeout.get(); + } + + int getActiveRequestsCountTimeout() { return activeRequestsCountTimeout.get(); } + +} \ No newline at end of file diff --git a/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/LoadBalancerStats.java b/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/LoadBalancerStats.java index 427610de..2e409f80 100644 --- a/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/LoadBalancerStats.java +++ b/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/LoadBalancerStats.java @@ -17,17 +17,13 @@ */ package com.netflix.loadbalancer; -import com.google.common.base.Preconditions; + import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; import com.google.common.cache.RemovalListener; import com.netflix.client.IClientConfigAware; -import com.netflix.client.config.ClientConfigFactory; -import com.netflix.client.config.CommonClientConfigKey; import com.netflix.client.config.IClientConfig; -import com.netflix.client.config.IClientConfigKey; -import com.netflix.client.config.UnboxedIntProperty; import com.netflix.servo.annotations.DataSourceType; import com.netflix.servo.annotations.Monitor; import com.netflix.servo.monitor.Monitors; @@ -58,39 +54,13 @@ public class LoadBalancerStats implements IClientConfigAware { private static final String PREFIX = "LBStats_"; - public static final IClientConfigKey ACTIVE_REQUESTS_COUNT_TIMEOUT = new CommonClientConfigKey( - "niws.loadbalancer.serverStats.activeRequestsCount.effectiveWindowSeconds", 60 * 10) {}; - - public static final IClientConfigKey CONNECTION_FAILURE_COUNT_THRESHOLD = new CommonClientConfigKey( - "niws.loadbalancer.%s.connectionFailureCountThreshold", 3) {}; - - public static final IClientConfigKey CIRCUIT_TRIP_TIMEOUT_FACTOR_SECONDS = new CommonClientConfigKey( - "niws.loadbalancer.%s.circuitTripTimeoutFactorSeconds", 10) {}; - - public static final IClientConfigKey CIRCUIT_TRIP_MAX_TIMEOUT_SECONDS = new CommonClientConfigKey( - "niws.loadbalancer.%s.circuitTripMaxTimeoutSeconds", 30) {}; - - public static final IClientConfigKey DEFAULT_CONNECTION_FAILURE_COUNT_THRESHOLD = new CommonClientConfigKey( - "niws.loadbalancer.default.connectionFailureCountThreshold", 3) {}; - - public static final IClientConfigKey DEFAULT_CIRCUIT_TRIP_TIMEOUT_FACTOR_SECONDS = new CommonClientConfigKey( - "niws.loadbalancer.default.circuitTripTimeoutFactorSeconds", 10) {}; - - public static final IClientConfigKey DEFAULT_CIRCUIT_TRIP_MAX_TIMEOUT_SECONDS = new CommonClientConfigKey( - "niws.loadbalancer.default.circuitTripMaxTimeoutSeconds", 30) {}; private String name; + private ConfigStats configStats = new ConfigStats(); volatile Map zoneStatsMap = new ConcurrentHashMap<>(); volatile Map> upServerListZoneMap = new ConcurrentHashMap<>(); - - private UnboxedIntProperty connectionFailureThreshold = new UnboxedIntProperty(CONNECTION_FAILURE_COUNT_THRESHOLD.defaultValue()); - - private UnboxedIntProperty circuitTrippedTimeoutFactor = new UnboxedIntProperty(CIRCUIT_TRIP_TIMEOUT_FACTOR_SECONDS.defaultValue()); - - private UnboxedIntProperty maxCircuitTrippedTimeout = new UnboxedIntProperty(CIRCUIT_TRIP_MAX_TIMEOUT_SECONDS.defaultValue()); - private UnboxedIntProperty activeRequestsCountTimeout = new UnboxedIntProperty(ACTIVE_REQUESTS_COUNT_TIMEOUT.defaultValue()); private final LoadingCache serverStatsCache = CacheBuilder.newBuilder() .expireAfterAccess(30, TimeUnit.MINUTES) @@ -102,7 +72,7 @@ public ServerStats load(Server server) { }); protected ServerStats createServerStats(Server server) { - ServerStats ss = new ServerStats(this); + ServerStats ss = new ServerStats(configStats); //configure custom settings ss.setBufferSize(1000); ss.setPublishInterval(1000); @@ -123,21 +93,7 @@ public LoadBalancerStats(String name) { @Override public void initWithNiwsConfig(IClientConfig clientConfig) { this.name = clientConfig.getClientName(); - Preconditions.checkArgument(name != null, "IClientConfig#getCLientName() must not be null"); - this.connectionFailureThreshold = new UnboxedIntProperty( - clientConfig.getGlobalProperty(CONNECTION_FAILURE_COUNT_THRESHOLD.format(name)) - .fallbackWith(clientConfig.getGlobalProperty(DEFAULT_CONNECTION_FAILURE_COUNT_THRESHOLD)) - ); - this.circuitTrippedTimeoutFactor = new UnboxedIntProperty( - clientConfig.getGlobalProperty(CIRCUIT_TRIP_TIMEOUT_FACTOR_SECONDS.format(name)) - .fallbackWith(clientConfig.getGlobalProperty(DEFAULT_CIRCUIT_TRIP_TIMEOUT_FACTOR_SECONDS)) - ); - this.maxCircuitTrippedTimeout = new UnboxedIntProperty( - clientConfig.getGlobalProperty(CIRCUIT_TRIP_MAX_TIMEOUT_SECONDS.format(name)) - .fallbackWith(clientConfig.getGlobalProperty(DEFAULT_CIRCUIT_TRIP_MAX_TIMEOUT_SECONDS)) - ); - this.activeRequestsCountTimeout = new UnboxedIntProperty( - clientConfig.getGlobalProperty(ACTIVE_REQUESTS_COUNT_TIMEOUT)); + this.configStats.initWithNiwsConfig(clientConfig); } @@ -149,23 +105,6 @@ public void setName(String name) { this.name = name; } - UnboxedIntProperty getConnectionFailureCountThreshold() { - return connectionFailureThreshold; - - } - - UnboxedIntProperty getCircuitTrippedTimeoutFactor() { - return circuitTrippedTimeoutFactor; - } - - UnboxedIntProperty getCircuitTripMaxTimeoutSeconds() { - return maxCircuitTrippedTimeout; - } - - UnboxedIntProperty getActiveRequestsCountTimeout() { - return activeRequestsCountTimeout; - } - /** * The caller o this class is tasked to call this method every so often if * the servers participating in the LoadBalancer changes diff --git a/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/ServerStats.java b/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/ServerStats.java index 44a2c2f1..cde75019 100644 --- a/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/ServerStats.java +++ b/ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/ServerStats.java @@ -19,10 +19,6 @@ import com.google.common.annotations.VisibleForTesting; -import com.netflix.client.config.CommonClientConfigKey; -import com.netflix.client.config.IClientConfigKey; -import com.netflix.client.config.Property; -import com.netflix.client.config.UnboxedIntProperty; import com.netflix.servo.annotations.DataSourceType; import com.netflix.servo.annotations.Monitor; import com.netflix.stats.distribution.DataDistribution; @@ -45,10 +41,7 @@ public class ServerStats { private static final int DEFAULT_PUBLISH_INTERVAL = 60 * 1000; // = 1 minute private static final int DEFAULT_BUFFER_SIZE = 60 * 1000; // = 1000 requests/sec for 1 minute - private final UnboxedIntProperty connectionFailureThreshold; - private final UnboxedIntProperty circuitTrippedTimeoutFactor; - private final UnboxedIntProperty maxCircuitTrippedTimeout; - private final UnboxedIntProperty activeRequestsCountTimeout; + private final ConfigStats configStats; private static final double[] PERCENTS = makePercentValues(); @@ -85,17 +78,11 @@ public class ServerStats { private volatile long firstConnectionTimestamp = 0; public ServerStats() { - connectionFailureThreshold = new UnboxedIntProperty(Property.of(LoadBalancerStats.CONNECTION_FAILURE_COUNT_THRESHOLD.defaultValue())); - circuitTrippedTimeoutFactor = new UnboxedIntProperty(LoadBalancerStats.CIRCUIT_TRIP_TIMEOUT_FACTOR_SECONDS.defaultValue()); - maxCircuitTrippedTimeout = new UnboxedIntProperty(LoadBalancerStats.CIRCUIT_TRIP_MAX_TIMEOUT_SECONDS.defaultValue()); - activeRequestsCountTimeout = new UnboxedIntProperty(LoadBalancerStats.ACTIVE_REQUESTS_COUNT_TIMEOUT.defaultValue()); + configStats = new ConfigStats(); } - public ServerStats(LoadBalancerStats lbStats) { - maxCircuitTrippedTimeout = lbStats.getCircuitTripMaxTimeoutSeconds(); - circuitTrippedTimeoutFactor = lbStats.getCircuitTrippedTimeoutFactor(); - connectionFailureThreshold = lbStats.getConnectionFailureCountThreshold(); - activeRequestsCountTimeout = lbStats.getActiveRequestsCountTimeout(); + public ServerStats(ConfigStats cfgStats) { + configStats = cfgStats; } /** @@ -241,7 +228,7 @@ public int getActiveRequestsCount(long currentTime) { int count = activeRequestsCount.get(); if (count == 0) { return 0; - } else if (currentTime - lastActiveRequestsCountChangeTimestamp > activeRequestsCountTimeout.get() * 1000 || count < 0) { + } else if (currentTime - lastActiveRequestsCountChangeTimestamp > configStats.getActiveRequestsCountTimeout() * 1000 || count < 0) { activeRequestsCount.set(0); return 0; } else { @@ -285,14 +272,14 @@ private long getCircuitBreakerTimeout() { private long getCircuitBreakerBlackoutPeriod() { int failureCount = successiveConnectionFailureCount.get(); - int threshold = connectionFailureThreshold.get(); + int threshold = configStats.getConnectionFailureThreshold(); if (failureCount < threshold) { return 0; } int diff = Math.min(failureCount - threshold, 16); - int blackOutSeconds = (1 << diff) * circuitTrippedTimeoutFactor.get(); - if (blackOutSeconds > maxCircuitTrippedTimeout.get()) { - blackOutSeconds = maxCircuitTrippedTimeout.get(); + int blackOutSeconds = (1 << diff) * configStats.getCircuitTrippedTimeoutFactor(); + if (blackOutSeconds > configStats.getMaxCircuitTrippedTimeout()) { + blackOutSeconds = configStats.getMaxCircuitTrippedTimeout(); } return blackOutSeconds * 1000L; }