From 4531ad31c0edcc1c89b1f9befc1b3814d8ec1add Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Tue, 16 Aug 2022 15:52:28 -0700 Subject: [PATCH] Various review comment fixes. --- .../util/OutlierDetectionLoadBalancer.java | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java b/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java index e343d0e366fe..b05c6dc2daf7 100644 --- a/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java +++ b/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java @@ -247,7 +247,8 @@ public void start(SubchannelStateListener listener) { @Override public Attributes getAttributes() { if (addressTracker != null) { - return delegate.getAttributes().toBuilder().set(ADDRESS_TRACKER_ATTR_KEY, addressTracker).build(); + return delegate.getAttributes().toBuilder().set(ADDRESS_TRACKER_ATTR_KEY, addressTracker) + .build(); } else { return delegate.getAttributes(); } @@ -654,6 +655,9 @@ void maybeUnejectOutliers(Long detectionTimerStartNanos) { * with the next ejection. */ double nextEjectionPercentage() { + if (trackerMap.isEmpty()) { + return 0; + } int totalAddresses = 0; int ejectedAddresses = 0; for (AddressTracker tracker : trackerMap.values()) { @@ -672,12 +676,10 @@ void maybeUnejectOutliers(Long detectionTimerStartNanos) { */ interface OutlierEjectionAlgorithm { - /** - * Is the given {@link EquivalentAddressGroup} an outlier based on the past call results stored - * in {@link AddressTracker}. - */ + /** Eject any outlier addresses. */ void ejectOutliers(AddressTrackerMap trackerMap, long ejectionTimeMillis); + /** Builds a list of algorithms that are enabled in the given config. */ @Nullable static List forConfig(OutlierDetectionLoadBalancerConfig config) { ImmutableList.Builder algoListBuilder = ImmutableList.builder(); @@ -724,15 +726,16 @@ public void ejectOutliers(AddressTrackerMap trackerMap, long ejectionTimeNanos) double mean = mean(successRates); double stdev = standardDeviation(successRates, mean); + double requiredSuccessRate = + mean - stdev * (config.successRateEjection.stdevFactor / 1000f); + for (AddressTracker tracker : trackersWithVolume) { - // If an ejection now would take us past the max configured ejection percentagem stop here. + // If an ejection now would take us past the max configured ejection percentage, stop here. if (trackerMap.nextEjectionPercentage() > config.maxEjectionPercent) { return; } // If success rate is below the threshold, eject the address. - double requiredSuccessRate = - mean - stdev * (config.successRateEjection.stdevFactor / 1000f); if (tracker.successRate() < requiredSuccessRate) { // Only eject some addresses based on the enforcement percentage. if (new Random().nextInt(100) < config.successRateEjection.enforcementPercentage) { @@ -896,7 +899,7 @@ public Builder setMaxEjectionPercent(Integer maxEjectionPercent) { return this; } - /** Set to enable success rate eejction. */ + /** Set to enable success rate ejection. */ public Builder setSuccessRateEjection( SuccessRateEjection successRateEjection) { this.successRateEjection = successRateEjection; @@ -962,6 +965,7 @@ public Builder setStdevFactor(Integer stdevFactor) { /** Only eject this percentage of outliers. */ public Builder setEnforcementPercentage(Integer enforcementPercentage) { checkArgument(enforcementPercentage != null); + checkArgument(enforcementPercentage >= 0 && enforcementPercentage <= 100); this.enforcementPercentage = enforcementPercentage; return this; } @@ -969,6 +973,7 @@ public Builder setEnforcementPercentage(Integer enforcementPercentage) { /** The minimum amount of hosts needed for success rate ejection. */ public Builder setMinimumHosts(Integer minimumHosts) { checkArgument(minimumHosts != null); + checkArgument(minimumHosts >= 0); this.minimumHosts = minimumHosts; return this; } @@ -976,6 +981,7 @@ public Builder setMinimumHosts(Integer minimumHosts) { /** The minimum address request volume to be considered for success rate ejection. */ public Builder setRequestVolume(Integer requestVolume) { checkArgument(requestVolume != null); + checkArgument(requestVolume >= 0); this.requestVolume = requestVolume; return this; } @@ -1013,6 +1019,7 @@ public static class Builder { /** The failure percentage that will result in an address being considered an outlier. */ public Builder setThreshold(Integer threshold) { checkArgument(threshold != null); + checkArgument(threshold >= 0 && threshold <= 100); this.threshold = threshold; return this; } @@ -1020,6 +1027,7 @@ public Builder setThreshold(Integer threshold) { /** Only eject this percentage of outliers. */ public Builder setEnforcementPercentage(Integer enforcementPercentage) { checkArgument(enforcementPercentage != null); + checkArgument(enforcementPercentage >= 0 && enforcementPercentage <= 100); this.enforcementPercentage = enforcementPercentage; return this; } @@ -1027,6 +1035,7 @@ public Builder setEnforcementPercentage(Integer enforcementPercentage) { /** The minimum amount of host for failure percentage ejection to be enabled. */ public Builder setMinimumHosts(Integer minimumHosts) { checkArgument(minimumHosts != null); + checkArgument(minimumHosts >= 0); this.minimumHosts = minimumHosts; return this; } @@ -1037,6 +1046,7 @@ public Builder setMinimumHosts(Integer minimumHosts) { */ public Builder setRequestVolume(Integer requestVolume) { checkArgument(requestVolume != null); + checkArgument(requestVolume >= 0); this.requestVolume = requestVolume; return this; } @@ -1049,11 +1059,8 @@ public FailurePercentageEjection build() { } } - /** - * Determine if outlier detection is at all enabled in this config. - */ + /** Determine if any outlier detection algorithms are enabled in the config. */ boolean outlierDetectionEnabled() { - // One of the two supported algorithms needs to be configured. return successRateEjection != null || failurePercentageEjection != null; } }