Centralize qos checks in Responses.java

palantir · Oct 7, 2024 · 1147b99 · 1147b99
1 parent 8508f39
commit 1147b99
Show file tree

Hide file tree

Showing 5 changed files with 33 additions and 18 deletions.
diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/BalancedScoreTracker.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/BalancedScoreTracker.java
@@ -155,6 +155,11 @@ public void undoStartRequest() {
         public void onSuccess(Response response) {
             inflight.decrementAndGet();
 
+            if (Responses.isQosDueToCustom(response)) {
+                // The server has marked this QoS exception as something that the balanced score
+                // tracker cannot understand.
+                return;
+            }
             if (isGlobalQosStatus(response) || Responses.isServerErrorRange(response)) {
                 recentFailuresReservoir.update(FAILURE_WEIGHT);
                 observability.debugLogStatusFailure(response);

diff --git a/...java/com/palantir/dialogue/core/CautiousIncreaseAggressiveDecreaseConcurrencyLimiter.java b/...java/com/palantir/dialogue/core/CautiousIncreaseAggressiveDecreaseConcurrencyLimiter.java
@@ -19,8 +19,6 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.util.concurrent.AtomicDouble;
 import com.google.common.util.concurrent.FutureCallback;
-import com.palantir.conjure.java.api.errors.QosReason;
-import com.palantir.conjure.java.api.errors.QosReason.DueTo;
 import com.palantir.dialogue.Response;
 import com.palantir.dialogue.core.LimitedChannel.LimitEnforcement;
 import com.palantir.logsafe.SafeArg;
@@ -105,8 +103,8 @@ void onSuccess(Response result, PermitControl control) {
                     control.ignore();
                 } else if (Responses.isTooManyRequests(result)) {
                     control.ignore();
-                } else if ((Responses.isQosStatus(result))) {
-                    if (isQosDueToCustom(result)) {
+                } else if (Responses.isQosStatus(result)) {
+                    if (Responses.isQosDueToCustom(result)) {
                         control.ignore();
                     } else {
                         control.dropped();
@@ -131,7 +129,7 @@ void onFailure(Throwable throwable, PermitControl control) {
             @Override
             void onSuccess(Response result, PermitControl control) {
                 if (Responses.isTooManyRequests(result)) {
-                    if (isQosDueToCustom(result)) {
+                    if (Responses.isQosDueToCustom(result)) {
                         control.ignore();
                     } else {
                         control.dropped();
@@ -167,11 +165,6 @@ void onFailure(Throwable _throwable, PermitControl control) {
         abstract void onFailure(Throwable throwable, PermitControl control);
     }
 
-    private static boolean isQosDueToCustom(Response result) {
-        QosReason reason = DialogueQosReasonDecoder.parse(result);
-        return DueTo.CUSTOM.equals(reason.dueTo().orElse(null));
-    }
-
     interface PermitControl {
 
         void ignore();

diff --git a/...e/src/main/java/com/palantir/dialogue/core/PinUntilErrorNodeSelectionStrategyChannel.java b/...e/src/main/java/com/palantir/dialogue/core/PinUntilErrorNodeSelectionStrategyChannel.java
@@ -147,7 +147,9 @@ public void onSuccess(Response response) {
                 // workflows where it is important for a large number of requests to all land on the same node,
                 // even if a couple of them get rate limited in the middle.
                 if (Responses.isServerErrorRange(response)
-                        || (Responses.isQosStatus(response) && !Responses.isTooManyRequests(response))) {
+                        || (Responses.isQosStatus(response)
+                                && !Responses.isQosDueToCustom(response)
+                                && !Responses.isTooManyRequests(response))) {
                     OptionalInt next = incrementHostIfNecessary(pin);
                     instrumentation.receivedErrorStatus(pin, channel, response, next);
                 } else {

diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/Responses.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/Responses.java
@@ -16,6 +16,9 @@
 package com.palantir.dialogue.core;
 
 import com.google.common.net.HttpHeaders;
+import com.palantir.conjure.java.api.errors.QosReason;
+import com.palantir.conjure.java.api.errors.QosReason.DueTo;
+import com.palantir.conjure.java.api.errors.QosReason.RetryHint;
 import com.palantir.dialogue.Response;
 
 /** Utility functionality for {@link Response} handling. */
@@ -41,6 +44,22 @@ static boolean isQosStatus(Response response) {
         return isRetryOther(response) || isTooManyRequests(response) || isUnavailable(response);
     }
 
+    static boolean isQosDueToCustom(Response result) {
+        if (!isQosStatus(result)) {
+            return false;
+        }
+        QosReason reason = DialogueQosReasonDecoder.parse(result);
+        return DueTo.CUSTOM.equals(reason.dueTo().orElse(null));
+    }
+
+    static boolean isRetryableQos(Response result) {
+        if (!isQosStatus(result)) {
+            return false;
+        }
+        QosReason reason = DialogueQosReasonDecoder.parse(result);
+        return !RetryHint.PROPAGATE.equals(reason.retryHint().orElse(null));
+    }
+
     static boolean isServerErrorRange(Response response) {
         return response.code() / 100 == 5;
     }

diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/RetryingChannel.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/RetryingChannel.java
@@ -26,7 +26,6 @@
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.common.util.concurrent.SettableFuture;
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
-import com.palantir.conjure.java.api.errors.QosReason.RetryHint;
 import com.palantir.conjure.java.client.config.ClientConfiguration;
 import com.palantir.dialogue.Endpoint;
 import com.palantir.dialogue.EndpointChannel;
@@ -382,15 +381,12 @@ private long getBackoffNanoseconds() {
         private boolean isRetryableQosStatus(Response response) {
             switch (serverQoS) {
                 case AUTOMATIC_RETRY:
-                    return Responses.isQosStatus(response)
-                            && RetryHint.PROPAGATE
-                                    != DialogueQosReasonDecoder.parse(response)
-                                            .retryHint()
-                                            .orElse(null);
+                    return Responses.isRetryableQos(response);
                 case PROPAGATE_429_and_503_TO_CALLER:
                     return Responses.isQosStatus(response)
                             && !Responses.isTooManyRequests(response)
-                            && !Responses.isUnavailable(response);
+                            && !Responses.isUnavailable(response)
+                            && Responses.isRetryableQos(response);
             }
             throw new SafeIllegalStateException(
                     "Encountered unknown propagate QoS configuration", SafeArg.of("serverQoS", serverQoS));