diff --git a/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java b/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java index 6319fd6010a..c11a174efe2 100644 --- a/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java +++ b/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java @@ -24,6 +24,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ForwardingMap; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import io.grpc.Attributes; @@ -164,8 +165,9 @@ public void run() { trackerMap.swapCounters(); - OutlierEjectionAlgorithm.forConfig(config) - .ejectOutliers(trackerMap, detectionTimerStartNanos); + for (OutlierEjectionAlgorithm algo : OutlierEjectionAlgorithm.forConfig(config)) { + algo.ejectOutliers(trackerMap, detectionTimerStartNanos); + } trackerMap.maybeUnejectOutliers(detectionTimerStartNanos); } @@ -649,14 +651,15 @@ interface OutlierEjectionAlgorithm { void ejectOutliers(AddressTrackerMap trackerMap, long ejectionTimeMillis); @Nullable - static OutlierEjectionAlgorithm forConfig(OutlierDetectionLoadBalancerConfig config) { + static List forConfig(OutlierDetectionLoadBalancerConfig config) { + ImmutableList.Builder algoListBuilder = ImmutableList.builder(); if (config.successRateEjection != null) { - return new SuccessRateOutlierEjectionAlgorithm(config); - } else if (config.failurePercentageEjection != null) { - return new FailurePercentageOutlierEjectionAlgorithm(config); - } else { - return null; + algoListBuilder.add(new SuccessRateOutlierEjectionAlgorithm(config)); } + if (config.failurePercentageEjection != null) { + algoListBuilder.add(new FailurePercentageOutlierEjectionAlgorithm(config)); + } + return algoListBuilder.build(); } } @@ -752,7 +755,7 @@ public void ejectOutliers(AddressTrackerMap trackerMap, long ejectionTimeMillis) // If this address does not have enough volume to be considered, skip to the next one. for (AddressTracker tracker : trackerMap.values()) { - // 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; } diff --git a/core/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java b/core/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java index 2aeaf71c4b4..407cd1071b4 100644 --- a/core/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java +++ b/core/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java @@ -810,6 +810,86 @@ public void subchannelUpdateAddress_multipleReplacedWithSingle() { assertThat(subchannel.isEjected()).isFalse(); } + /** Both algorithms configured, but no outliers. */ + @Test + public void successRateAndFailurePercentage_noOutliers() { + OutlierDetectionLoadBalancerConfig config = new OutlierDetectionLoadBalancerConfig.Builder() + .setMaxEjectionPercent(50) + .setSuccessRateEjection( + new SuccessRateEjection.Builder() + .setMinimumHosts(3) + .setRequestVolume(10).build()) + .setFailurePercentageEjection( + new FailurePercentageEjection.Builder() + .setMinimumHosts(3) + .setRequestVolume(10).build()) + .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)).build(); + + loadBalancer.handleResolvedAddresses(buildResolvedAddress(config, servers)); + + generateLoad(ImmutableMap.of()); + + // Move forward in time to a point where the detection timer has fired. + fakeClock.forwardTime(config.intervalSecs + 1, TimeUnit.SECONDS); + + // No outliers, no ejections. + assertEjectedSubchannels(ImmutableSet.of()); + } + + /** Both algorithms configured, success rate detects an outlier. */ + @Test + public void successRateAndFailurePercentage_successRateOutlier() { + OutlierDetectionLoadBalancerConfig config = new OutlierDetectionLoadBalancerConfig.Builder() + .setMaxEjectionPercent(50) + .setSuccessRateEjection( + new SuccessRateEjection.Builder() + .setMinimumHosts(3) + .setRequestVolume(10).build()) + .setFailurePercentageEjection( + new FailurePercentageEjection.Builder() + .setMinimumHosts(3) + .setRequestVolume(10) + .setEnforcementPercentage(0).build()) // Configured, but not enforcing. + .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)).build(); + + loadBalancer.handleResolvedAddresses(buildResolvedAddress(config, servers)); + + generateLoad(ImmutableMap.of(subchannel1, Status.DEADLINE_EXCEEDED)); + + // Move forward in time to a point where the detection timer has fired. + fakeClock.forwardTime(config.intervalSecs + 1, TimeUnit.SECONDS); + + // The one subchannel that was returning errors should be ejected. + assertEjectedSubchannels(ImmutableSet.of(servers.get(0))); + } + + /** Both algorithms configured, error percentage detects an outlier. */ + @Test + public void successRateAndFailurePercentage_errorPercentageOutlier() { + OutlierDetectionLoadBalancerConfig config = new OutlierDetectionLoadBalancerConfig.Builder() + .setMaxEjectionPercent(50) + .setSuccessRateEjection( + new SuccessRateEjection.Builder() + .setMinimumHosts(3) + .setRequestVolume(10) + .setEnforcementPercentage(0).build()) + .setFailurePercentageEjection( + new FailurePercentageEjection.Builder() + .setMinimumHosts(3) + .setRequestVolume(10).build()) // Configured, but not enforcing. + .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)).build(); + + loadBalancer.handleResolvedAddresses(buildResolvedAddress(config, servers)); + + generateLoad(ImmutableMap.of(subchannel1, Status.DEADLINE_EXCEEDED)); + + // Move forward in time to a point where the detection timer has fired. + fakeClock.forwardTime(config.intervalSecs + 1, TimeUnit.SECONDS); + + // The one subchannel that was returning errors should be ejected. + assertEjectedSubchannels(ImmutableSet.of(servers.get(0))); + } + @Test public void mathChecksOut() { ImmutableList values = ImmutableList.of(600d, 470d, 170d, 430d, 300d);