Various review comment fixes.

grpc · Aug 16, 2022 · 4cc9c8d · 4cc9c8d
1 parent 605d8e5
commit 4cc9c8d
Showing 1 changed file with 18 additions and 12 deletions.
diff --git a/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java b/core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java
@@ -654,6 +654,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 +675,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<OutlierEjectionAlgorithm> forConfig(OutlierDetectionLoadBalancerConfig config) {
       ImmutableList.Builder<OutlierEjectionAlgorithm> algoListBuilder = ImmutableList.builder();
@@ -724,15 +725,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 +898,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,20 +964,23 @@ 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;
         }
 
         /** 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;
         }
 
         /** 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,20 +1018,23 @@ 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;
         }
 
         /** Only eject this percentage of outliers. */
         public Builder setEnforcementPercentage(Integer enforcementPercentage) {
           checkArgument(enforcementPercentage != null);
+          checkArgument(enforcementPercentage >= 0 && enforcementPercentage <= 100);
           this.enforcementPercentage = enforcementPercentage;
           return this;
         }
 
         /** 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 +1045,7 @@ public Builder setMinimumHosts(Integer minimumHosts) {
          */
         public Builder setRequestVolume(Integer requestVolume) {
           checkArgument(requestVolume != null);
+          checkArgument(requestVolume >= 0);
           this.requestVolume = requestVolume;
           return this;
         }
@@ -1049,11 +1058,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;
     }
   }