diff --git a/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java b/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java index 12d71ba215..d6ada1dbd1 100644 --- a/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java +++ b/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java @@ -713,7 +713,8 @@ public final class OutlierDetectionLoadBalancer extends LoadBalancer { public void ejectOutliers(AddressTrackerMap trackerMap, long ejectionTimeNanos) { // Only consider addresses that have the minimum request volume specified in the config. - List trackersWithVolume = trackersWithVolume(trackerMap, config); + List trackersWithVolume = trackersWithVolume(trackerMap, + config.successRateEjection.requestVolume); // If we don't have enough addresses with significant volume then there's nothing to do. if (trackersWithVolume.size() < config.successRateEjection.minimumHosts || trackersWithVolume.size() == 0) { @@ -749,18 +750,6 @@ public final class OutlierDetectionLoadBalancer extends LoadBalancer { } } - /** Returns only the trackers that have the minimum configured volume to be considered. */ - private List trackersWithVolume(AddressTrackerMap trackerMap, - OutlierDetectionLoadBalancerConfig config) { - List trackersWithVolume = new ArrayList<>(); - for (AddressTracker tracker : trackerMap.values()) { - if (tracker.inactiveVolume() >= config.successRateEjection.requestVolume) { - trackersWithVolume.add(tracker); - } - } - return trackersWithVolume; - } - /** Calculates the mean of the given values. */ @VisibleForTesting static double mean(Collection values) { @@ -797,13 +786,17 @@ public final class OutlierDetectionLoadBalancer extends LoadBalancer { @Override public void ejectOutliers(AddressTrackerMap trackerMap, long ejectionTimeNanos) { - // If we don't have the minimum amount of addresses the config calls for, then return. - if (trackerMap.size() < config.failurePercentageEjection.minimumHosts) { + // Only consider addresses that have the minimum request volume specified in the config. + List trackersWithVolume = trackersWithVolume(trackerMap, + config.failurePercentageEjection.requestVolume); + // If we don't have enough addresses with significant volume then there's nothing to do. + if (trackersWithVolume.size() < config.failurePercentageEjection.minimumHosts + || trackersWithVolume.size() == 0) { return; } // If this address does not have enough volume to be considered, skip to the next one. - for (AddressTracker tracker : trackerMap.values()) { + for (AddressTracker tracker : trackersWithVolume) { // If we are above the max ejection percentage, don't eject any more. This will allow the // total ejections to go one above the max, but at the same time it assures at least one // ejection, which the spec calls for. This behavior matches what Envoy proxy does. @@ -827,6 +820,18 @@ public final class OutlierDetectionLoadBalancer extends LoadBalancer { } } + /** Returns only the trackers that have the minimum configured volume to be considered. */ + private static List trackersWithVolume(AddressTrackerMap trackerMap, + int volume) { + List trackersWithVolume = new ArrayList<>(); + for (AddressTracker tracker : trackerMap.values()) { + if (tracker.inactiveVolume() >= volume) { + trackersWithVolume.add(tracker); + } + } + return trackersWithVolume; + } + /** Counts how many addresses are in a given address group. */ private static boolean hasSingleAddress(List addressGroups) { int addressCount = 0; diff --git a/core/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java b/core/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java index 94426d2c9d..b60c685fa5 100644 --- a/core/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java +++ b/core/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java @@ -496,7 +496,8 @@ public class OutlierDetectionLoadBalancerTest { } /** - * The success rate algorithm does not apply if enough addresses have the required volume. + * The success rate algorithm does not apply if we don't have enough addresses that have the + * required volume. */ @Test public void successRateOneOutlier_notEnoughAddressesWithVolume() { @@ -504,13 +505,17 @@ public class OutlierDetectionLoadBalancerTest { .setMaxEjectionPercent(50) .setSuccessRateEjection( new SuccessRateEjection.Builder() - .setMinimumHosts(6) // We don't have this many hosts... - .setRequestVolume(10).build()) + .setMinimumHosts(5) + .setRequestVolume(20).build()) .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)).build(); loadBalancer.handleResolvedAddresses(buildResolvedAddress(config, servers)); - generateLoad(ImmutableMap.of(subchannel1, Status.DEADLINE_EXCEEDED), 7); + generateLoad( + ImmutableMap.of(subchannel1, Status.DEADLINE_EXCEEDED), + // subchannel2 has only 19 calls which results in success rate not triggering. + ImmutableMap.of(subchannel2, 19), + 7); // Move forward in time to a point where the detection timer has fired. forwardTime(config); @@ -682,6 +687,35 @@ public class OutlierDetectionLoadBalancerTest { assertEjectedSubchannels(ImmutableSet.of()); } + /** + * The failure percentage algorithm does not apply if we don't have enough addresses that have the + * required volume. + */ + @Test + public void failurePercentageOneOutlier_notEnoughAddressesWithVolume() { + OutlierDetectionLoadBalancerConfig config = new OutlierDetectionLoadBalancerConfig.Builder() + .setMaxEjectionPercent(50) + .setFailurePercentageEjection( + new FailurePercentageEjection.Builder() + .setMinimumHosts(5) + .setRequestVolume(20).build()) + .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)).build(); + + loadBalancer.handleResolvedAddresses(buildResolvedAddress(config, servers)); + + generateLoad( + ImmutableMap.of(subchannel1, Status.DEADLINE_EXCEEDED), + // subchannel2 has only 19 calls which results in failure percentage not triggering. + ImmutableMap.of(subchannel2, 19), + 7); + + // Move forward in time to a point where the detection timer has fired. + forwardTime(config); + + // No subchannels should have been ejected. + assertEjectedSubchannels(ImmutableSet.of()); + } + /** * The enforcementPercentage configuration should be honored. */