From bfee2c7d3a0ba40408530fe3b51d259cc4be66a8 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Fri, 4 Oct 2024 13:49:13 -0400 Subject: [PATCH 1/9] parse QosReason data from remote servers respect RetryHint --- ...eAggressiveDecreaseConcurrencyLimiter.java | 18 +++++++--- .../core/DialogueQosReasonDecoder.java | 36 +++++++++++++++++++ .../dialogue/core/RetryingChannel.java | 7 +++- .../java/dialogue/serde/ErrorDecoder.java | 28 ++++++++++----- .../java/dialogue/serde/ErrorDecoderTest.java | 33 ++++++++++++++--- 5 files changed, 104 insertions(+), 18 deletions(-) create mode 100644 dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueQosReasonDecoder.java diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/CautiousIncreaseAggressiveDecreaseConcurrencyLimiter.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/CautiousIncreaseAggressiveDecreaseConcurrencyLimiter.java index 1679f9cef..02ecde58e 100644 --- a/dialogue-core/src/main/java/com/palantir/dialogue/core/CautiousIncreaseAggressiveDecreaseConcurrencyLimiter.java +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/CautiousIncreaseAggressiveDecreaseConcurrencyLimiter.java @@ -19,6 +19,8 @@ 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; @@ -99,8 +101,10 @@ enum Behavior { HOST_LEVEL() { @Override void onSuccess(Response result, PermitControl control) { - if (Responses.isTooManyRequests(result) || Responses.isInternalServerError(result)) { - // 429 or 500 + if (Responses.isTooManyRequests(result) + || Responses.isInternalServerError(result) + || isQosDueToCustom(result)) { + // 429, 500, or QoS due to a custom reason control.ignore(); } else if ((Responses.isQosStatus(result) && !Responses.isTooManyRequests(result)) || Responses.isServerErrorRange(result)) { @@ -123,8 +127,9 @@ void onFailure(Throwable throwable, PermitControl control) { ENDPOINT_LEVEL() { @Override void onSuccess(Response result, PermitControl control) { - if (Responses.isTooManyRequests(result) || Responses.isInternalServerError(result)) { - // 429 or 500 + if ((Responses.isTooManyRequests(result) && !isQosDueToCustom(result)) + || Responses.isInternalServerError(result)) { + // non-custom 429 or 500 control.dropped(); } else if (Responses.isServerErrorRange(result)) { // 501-599 @@ -156,6 +161,11 @@ 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 { /** diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueQosReasonDecoder.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueQosReasonDecoder.java new file mode 100644 index 000000000..f9d3268f9 --- /dev/null +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueQosReasonDecoder.java @@ -0,0 +1,36 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.dialogue.core; + +import com.palantir.conjure.java.api.errors.QosReason; +import com.palantir.conjure.java.api.errors.QosReasons; +import com.palantir.conjure.java.api.errors.QosReasons.QosResponseDecodingAdapter; +import com.palantir.dialogue.Response; +import java.util.Optional; + +enum DialogueQosReasonDecoder implements QosResponseDecodingAdapter { + INSTANCE; + + @Override + public Optional getFirstHeader(Response response, String headerName) { + return response.getFirstHeader(headerName); + } + + static QosReason parse(Response response) { + return QosReasons.parseFromResponse(response, DialogueQosReasonDecoder.INSTANCE); + } +} 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 index 04854cc55..85f908b4e 100644 --- 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,6 +26,7 @@ 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; @@ -381,7 +382,11 @@ private long getBackoffNanoseconds() { private boolean isRetryableQosStatus(Response response) { switch (serverQoS) { case AUTOMATIC_RETRY: - return Responses.isQosStatus(response); + return Responses.isQosStatus(response) + && RetryHint.PROPAGATE + != DialogueQosReasonDecoder.parse(response) + .retryHint() + .orElse(null); case PROPAGATE_429_and_503_TO_CALLER: return Responses.isQosStatus(response) && !Responses.isTooManyRequests(response) diff --git a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java index c73ec24ad..50642aea0 100644 --- a/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java +++ b/dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoder.java @@ -17,13 +17,14 @@ package com.palantir.conjure.java.dialogue.serde; import com.fasterxml.jackson.databind.ObjectMapper; -import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.io.CharStreams; import com.google.common.net.HttpHeaders; import com.google.common.primitives.Longs; import com.palantir.conjure.java.api.errors.QosException; import com.palantir.conjure.java.api.errors.QosReason; +import com.palantir.conjure.java.api.errors.QosReasons; +import com.palantir.conjure.java.api.errors.QosReasons.QosResponseDecodingAdapter; import com.palantir.conjure.java.api.errors.RemoteException; import com.palantir.conjure.java.api.errors.SerializableError; import com.palantir.conjure.java.api.errors.UnknownRemoteException; @@ -59,9 +60,6 @@ public enum ErrorDecoder { private static final SafeLogger log = SafeLoggerFactory.get(ErrorDecoder.class); private static final ObjectMapper MAPPER = ObjectMappers.newClientObjectMapper(); - @VisibleForTesting - static final QosReason QOS_REASON = QosReason.of("client-qos-response"); - public boolean isError(Response response) { return 300 <= response.code() && response.code() <= 599; } @@ -87,7 +85,8 @@ private RuntimeException decodeInternal(Response response) { String locationHeader = location.get(); try { UnknownRemoteException remoteException = new UnknownRemoteException(code, ""); - remoteException.initCause(QosException.retryOther(QOS_REASON, new URL(locationHeader))); + remoteException.initCause( + QosException.retryOther(qosReason(response), new URL(locationHeader))); return remoteException; } catch (MalformedURLException e) { log.error( @@ -104,10 +103,10 @@ private RuntimeException decodeInternal(Response response) { return response.getFirstHeader(HttpHeaders.RETRY_AFTER) .map(Longs::tryParse) .map(Duration::ofSeconds) - .map(duration -> QosException.throttle(QOS_REASON, duration)) - .orElseGet(() -> QosException.throttle(QOS_REASON)); + .map(duration -> QosException.throttle(qosReason(response), duration)) + .orElseGet(() -> QosException.throttle(qosReason(response))); case 503: - return QosException.unavailable(QOS_REASON); + return QosException.unavailable(qosReason(response)); } String body; @@ -189,4 +188,17 @@ public Throwable fillInStackTrace() { return this; } } + + private static QosReason qosReason(Response response) { + return QosReasons.parseFromResponse(response, DialogueQosResponseDecodingAdapter.INSTANCE); + } + + private enum DialogueQosResponseDecodingAdapter implements QosResponseDecodingAdapter { + INSTANCE; + + @Override + public Optional getFirstHeader(Response response, String headerName) { + return response.getFirstHeader(headerName); + } + } } diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java index 93b10fd87..51220de8d 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java @@ -25,6 +25,9 @@ import com.google.common.net.HttpHeaders; import com.palantir.conjure.java.api.errors.ErrorType; import com.palantir.conjure.java.api.errors.QosException; +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.conjure.java.api.errors.RemoteException; import com.palantir.conjure.java.api.errors.SerializableError; import com.palantir.conjure.java.api.errors.ServiceException; @@ -43,6 +46,7 @@ public final class ErrorDecoderTest { private static final ObjectMapper SERVER_MAPPER = ObjectMappers.newServerObjectMapper(); + private static final QosReason QOS_REASON = QosReason.of("client-qos-response"); private static final ServiceException SERVICE_EXCEPTION = new ServiceException(ErrorType.FAILED_PRECONDITION, SafeArg.of("key", "value")); @@ -97,7 +101,26 @@ public void testQos503() { RuntimeException result = decoder.decode(response); assertThat(result).isInstanceOfSatisfying(QosException.Unavailable.class, exception -> { - assertThat(exception.getReason()).isEqualTo(ErrorDecoder.QOS_REASON); + assertThat(exception.getReason()).isEqualTo(QOS_REASON); + }); + } + + @Test + public void testQos503WithMetadata() { + Response response = TestResponse.withBody(SERIALIZED_EXCEPTION) + .code(503) + .withHeader("Qos-Retry-Hint", "PROPAGATE") + .withHeader("Qos-Due-To", "CUSTOM"); + assertThat(decoder.isError(response)).isTrue(); + + RuntimeException result = decoder.decode(response); + assertThat(result).isInstanceOfSatisfying(QosException.Unavailable.class, exception -> { + assertThat(exception.getReason()) + .isEqualTo(QosReason.builder() + .from(QOS_REASON) + .dueTo(DueTo.CUSTOM) + .retryHint(RetryHint.PROPAGATE) + .build()); }); } @@ -108,7 +131,7 @@ public void testQos429() { RuntimeException result = decoder.decode(response); assertThat(result).isInstanceOfSatisfying(QosException.Throttle.class, exception -> { - assertThat(exception.getReason()).isEqualTo(ErrorDecoder.QOS_REASON); + assertThat(exception.getReason()).isEqualTo(QOS_REASON); assertThat(exception.getRetryAfter()).isEmpty(); }); } @@ -121,7 +144,7 @@ public void testQos429_retryAfter() { RuntimeException result = decoder.decode(response); assertThat(result).isInstanceOfSatisfying(QosException.Throttle.class, exception -> { - assertThat(exception.getReason()).isEqualTo(ErrorDecoder.QOS_REASON); + assertThat(exception.getReason()).isEqualTo(QOS_REASON); assertThat(exception.getRetryAfter()).hasValue(Duration.ofSeconds(3)); }); } @@ -134,7 +157,7 @@ public void testQos429_retryAfter_invalid() { RuntimeException result = decoder.decode(response); assertThat(result).isInstanceOfSatisfying(QosException.Throttle.class, exception -> { - assertThat(exception.getReason()).isEqualTo(ErrorDecoder.QOS_REASON); + assertThat(exception.getReason()).isEqualTo(QOS_REASON); assertThat(exception.getRetryAfter()).isEmpty(); }); } @@ -175,7 +198,7 @@ public void testQos308() { .isInstanceOf(UnknownRemoteException.class) .getRootCause() .isInstanceOfSatisfying(QosException.RetryOther.class, exception -> { - assertThat(exception.getReason()).isEqualTo(ErrorDecoder.QOS_REASON); + assertThat(exception.getReason()).isEqualTo(QOS_REASON); assertThat(exception.getRedirectTo()).asString().isEqualTo(expectedLocation); }); } From 091c45a84800f6d095f47e61e916584e22059f82 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 7 Oct 2024 16:13:59 -0400 Subject: [PATCH 2/9] Centralize qos checks in Responses.java --- .../dialogue/core/BalancedScoreTracker.java | 5 +++++ ...ntilErrorNodeSelectionStrategyChannel.java | 4 +++- .../com/palantir/dialogue/core/Responses.java | 19 +++++++++++++++++++ .../dialogue/core/RetryingChannel.java | 10 +++------- 4 files changed, 30 insertions(+), 8 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 index 2bfae50c5..96841717d 100644 --- 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/dialogue-core/src/main/java/com/palantir/dialogue/core/PinUntilErrorNodeSelectionStrategyChannel.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/PinUntilErrorNodeSelectionStrategyChannel.java index 2476519dd..e0e0f3a40 100644 --- a/dialogue-core/src/main/java/com/palantir/dialogue/core/PinUntilErrorNodeSelectionStrategyChannel.java +++ b/dialogue-core/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 index 1d5d19ce2..8e790e62d 100644 --- 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 index 85f908b4e..c869f0094 100644 --- 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)); From aa2da1ecffaea457d034e7c44af5c168568446f5 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 8 Oct 2024 11:26:53 -0400 Subject: [PATCH 3/9] update to cjr-api rc --- .../src/main/java/com/palantir/dialogue/core/Responses.java | 2 +- .../conjure/java/dialogue/serde/ErrorDecoderTest.java | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) 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 index 8e790e62d..a49b4e2ab 100644 --- a/dialogue-core/src/main/java/com/palantir/dialogue/core/Responses.java +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/Responses.java @@ -57,7 +57,7 @@ static boolean isRetryableQos(Response result) { return false; } QosReason reason = DialogueQosReasonDecoder.parse(result); - return !RetryHint.PROPAGATE.equals(reason.retryHint().orElse(null)); + return !RetryHint.DO_NOT_RETRY.equals(reason.retryHint().orElse(null)); } static boolean isServerErrorRange(Response response) { diff --git a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java index 51220de8d..9f26a7aa6 100644 --- a/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java +++ b/dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/ErrorDecoderTest.java @@ -109,8 +109,8 @@ public void testQos503() { public void testQos503WithMetadata() { Response response = TestResponse.withBody(SERIALIZED_EXCEPTION) .code(503) - .withHeader("Qos-Retry-Hint", "PROPAGATE") - .withHeader("Qos-Due-To", "CUSTOM"); + .withHeader("Qos-Retry-Hint", "do-not-retry") + .withHeader("Qos-Due-To", "custom"); assertThat(decoder.isError(response)).isTrue(); RuntimeException result = decoder.decode(response); @@ -119,7 +119,7 @@ public void testQos503WithMetadata() { .isEqualTo(QosReason.builder() .from(QOS_REASON) .dueTo(DueTo.CUSTOM) - .retryHint(RetryHint.PROPAGATE) + .retryHint(RetryHint.DO_NOT_RETRY) .build()); }); } From c9e71429f483e5e04a009be314dd83b301f84be8 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 8 Oct 2024 11:33:52 -0400 Subject: [PATCH 4/9] implicit dependency --- dialogue-core/build.gradle | 1 + 1 file changed, 1 insertion(+) diff --git a/dialogue-core/build.gradle b/dialogue-core/build.gradle index 50f31d559..ad386a123 100644 --- a/dialogue-core/build.gradle +++ b/dialogue-core/build.gradle @@ -17,6 +17,7 @@ dependencies { implementation 'com.palantir.tritium:tritium-metrics' implementation 'com.google.code.findbugs:jsr305' implementation 'com.google.errorprone:error_prone_annotations' + implementation 'com.palantir.conjure.java.api:errors' implementation 'com.palantir.conjure.java.api:service-config' implementation 'com.palantir.tracing:tracing-api' implementation 'com.palantir.refreshable:refreshable' From 10dcf2b3545a81baf9b4475febdb672878821969 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 8 Oct 2024 12:29:21 -0400 Subject: [PATCH 5/9] test coverage for concurrency limiters --- ...ressiveDecreaseConcurrencyLimiterTest.java | 97 ++++++++++++++----- .../com/palantir/dialogue/TestResponse.java | 4 - .../dialogue/TestResponseQosEncoder.java | 28 ++++++ 3 files changed, 103 insertions(+), 26 deletions(-) create mode 100644 dialogue-test-common/src/main/java/com/palantir/dialogue/TestResponseQosEncoder.java diff --git a/dialogue-core/src/test/java/com/palantir/dialogue/core/CautiousIncreaseAggressiveDecreaseConcurrencyLimiterTest.java b/dialogue-core/src/test/java/com/palantir/dialogue/core/CautiousIncreaseAggressiveDecreaseConcurrencyLimiterTest.java index b41eb7335..1855faff7 100644 --- a/dialogue-core/src/test/java/com/palantir/dialogue/core/CautiousIncreaseAggressiveDecreaseConcurrencyLimiterTest.java +++ b/dialogue-core/src/test/java/com/palantir/dialogue/core/CautiousIncreaseAggressiveDecreaseConcurrencyLimiterTest.java @@ -17,16 +17,19 @@ package com.palantir.dialogue.core; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.Uninterruptibles; +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.conjure.java.api.errors.QosReasons; import com.palantir.dialogue.Response; +import com.palantir.dialogue.TestResponse; +import com.palantir.dialogue.TestResponseQosEncoder; import com.palantir.dialogue.core.CautiousIncreaseAggressiveDecreaseConcurrencyLimiter.Behavior; import com.palantir.dialogue.core.CautiousIncreaseAggressiveDecreaseConcurrencyLimiter.Permit; import com.palantir.dialogue.core.LimitedChannel.LimitEnforcement; @@ -177,8 +180,7 @@ public void success_increasesLimitOnlyIfSufficientNumberOfRequestsAreInflight(Be @EnumSource(Behavior.class) public void onSuccess_releasesSuccessfully(Behavior behavior) { CautiousIncreaseAggressiveDecreaseConcurrencyLimiter limiter = limiter(behavior); - Response response = mock(Response.class); - when(response.code()).thenReturn(200); + Response response = new TestResponse().code(200); double max = limiter.getLimit(); limiter.acquire(LimitEnforcement.DEFAULT_ENABLED).get().onSuccess(response); @@ -188,9 +190,7 @@ public void onSuccess_releasesSuccessfully(Behavior behavior) { @Test public void onSuccess_dropsIfResponseIndicatesQosOrError_host_308() { CautiousIncreaseAggressiveDecreaseConcurrencyLimiter limiter = limiter(Behavior.HOST_LEVEL); - Response response = mock(Response.class); - when(response.getFirstHeader(eq("Location"))).thenReturn(Optional.of("https://localhost")); - when(response.code()).thenReturn(308); + Response response = new TestResponse().code(308).withHeader("Location", "https://localhost"); double max = limiter.getLimit(); limiter.acquire(LimitEnforcement.DEFAULT_ENABLED).get().onSuccess(response); @@ -202,8 +202,7 @@ public void onSuccess_successIfResponseIndicatesNonQos308() { // This represents google chunked-upload APIs which respond '308 Resume Incomplete' to indicate // a successful chunk upload request. CautiousIncreaseAggressiveDecreaseConcurrencyLimiter limiter = limiter(Behavior.HOST_LEVEL); - Response response = mock(Response.class); - when(response.code()).thenReturn(308); + Response response = new TestResponse().code(308); double max = limiter.getLimit(); limiter.acquire(LimitEnforcement.DEFAULT_ENABLED).get().onSuccess(response); @@ -213,32 +212,89 @@ public void onSuccess_successIfResponseIndicatesNonQos308() { @Test public void onSuccess_dropsIfResponseIndicatesQosOrError_host_503() { CautiousIncreaseAggressiveDecreaseConcurrencyLimiter limiter = limiter(Behavior.HOST_LEVEL); - Response response = mock(Response.class); - when(response.code()).thenReturn(503); + Response response = new TestResponse().code(503); double max = limiter.getLimit(); limiter.acquire(LimitEnforcement.DEFAULT_ENABLED).get().onSuccess(response); assertThat(limiter.getLimit()).as("For status %d", 503).isCloseTo(max * 0.9, Percentage.withPercentage(5)); } + @Test + public void onSuccess_ignoresIfResponseIndicatesQos_host_custom503() { + CautiousIncreaseAggressiveDecreaseConcurrencyLimiter limiter = limiter(Behavior.HOST_LEVEL); + TestResponse response = new TestResponse().code(503); + QosReason reason = + QosReason.builder().reason("reason").dueTo(DueTo.CUSTOM).build(); + QosReasons.encodeToResponse(reason, response, TestResponseQosEncoder.INSTANCE); + + double max = limiter.getLimit(); + limiter.acquire(LimitEnforcement.DEFAULT_ENABLED).get().onSuccess(response); + assertThat(limiter.getLimit()).as("For DueTo.CUSTOM status %d", 503).isEqualTo(max); + } + + @Test + public void onSuccess_ignoresIfResponseIndicatesQos_host_nonRetryable503() { + CautiousIncreaseAggressiveDecreaseConcurrencyLimiter limiter = limiter(Behavior.HOST_LEVEL); + TestResponse response = new TestResponse().code(503); + QosReason reason = QosReason.builder() + .reason("reason") + .retryHint(RetryHint.DO_NOT_RETRY) + .build(); + QosReasons.encodeToResponse(reason, response, TestResponseQosEncoder.INSTANCE); + + double max = limiter.getLimit(); + limiter.acquire(LimitEnforcement.DEFAULT_ENABLED).get().onSuccess(response); + assertThat(limiter.getLimit()) + .as("For status %d not impacted by RetryHint.DO_NOT_RETRY", 503) + .isCloseTo(max * 0.9, Percentage.withPercentage(5)); + } + @Test public void onSuccess_dropsIfResponseIndicatesQosOrError_endpoint() { CautiousIncreaseAggressiveDecreaseConcurrencyLimiter limiter = limiter(Behavior.ENDPOINT_LEVEL); int code = 429; - Response response = mock(Response.class); - when(response.code()).thenReturn(code); + Response response = new TestResponse().code(code); double max = limiter.getLimit(); limiter.acquire(LimitEnforcement.DEFAULT_ENABLED).get().onSuccess(response); assertThat(limiter.getLimit()).as("For status %d", code).isCloseTo(max * 0.9, Percentage.withPercentage(5)); } + @Test + public void onSuccess_ignoresIfResponseIndicatesQos_endpoint_custom429() { + CautiousIncreaseAggressiveDecreaseConcurrencyLimiter limiter = limiter(Behavior.ENDPOINT_LEVEL); + TestResponse response = new TestResponse().code(429); + QosReason reason = + QosReason.builder().reason("reason").dueTo(DueTo.CUSTOM).build(); + QosReasons.encodeToResponse(reason, response, TestResponseQosEncoder.INSTANCE); + + double max = limiter.getLimit(); + limiter.acquire(LimitEnforcement.DEFAULT_ENABLED).get().onSuccess(response); + assertThat(limiter.getLimit()).as("For DueTo.CUSTOM status %d", 429).isEqualTo(max); + } + + @Test + public void onSuccess_ignoresIfResponseIndicatesQos_endpoint_nonRetryable429() { + CautiousIncreaseAggressiveDecreaseConcurrencyLimiter limiter = limiter(Behavior.ENDPOINT_LEVEL); + TestResponse response = new TestResponse().code(429); + QosReason reason = QosReason.builder() + .reason("reason") + .retryHint(RetryHint.DO_NOT_RETRY) + .build(); + QosReasons.encodeToResponse(reason, response, TestResponseQosEncoder.INSTANCE); + + double max = limiter.getLimit(); + limiter.acquire(LimitEnforcement.DEFAULT_ENABLED).get().onSuccess(response); + assertThat(limiter.getLimit()) + .as("For status %d not impacted by RetryHint.DO_NOT_RETRY", 429) + .isCloseTo(max * 0.9, Percentage.withPercentage(5)); + } + @Test public void onSuccess_releasesSuccessfullyIfResponseIndicatesQosOrError_sticky() { CautiousIncreaseAggressiveDecreaseConcurrencyLimiter limiter = limiter(Behavior.STICKY); int code = 429; - Response response = mock(Response.class); - when(response.code()).thenReturn(code); + Response response = new TestResponse().code(code); double max = limiter.getLimit(); limiter.acquire(LimitEnforcement.DEFAULT_ENABLED).get().onSuccess(response); @@ -249,8 +305,7 @@ public void onSuccess_releasesSuccessfullyIfResponseIndicatesQosOrError_sticky() public void onSuccess_ignoresIfResponseIndicatesUnknownServerError_endpoint() { CautiousIncreaseAggressiveDecreaseConcurrencyLimiter limiter = limiter(Behavior.ENDPOINT_LEVEL); int code = 599; - Response response = mock(Response.class); - when(response.code()).thenReturn(code); + Response response = new TestResponse().code(code); double max = limiter.getLimit(); limiter.acquire(LimitEnforcement.DEFAULT_ENABLED).get().onSuccess(response); @@ -261,8 +316,7 @@ public void onSuccess_ignoresIfResponseIndicatesUnknownServerError_endpoint() { public void onSuccess_ignoresIfResponseIndicatesUnknownServerError_sticky() { CautiousIncreaseAggressiveDecreaseConcurrencyLimiter limiter = limiter(Behavior.STICKY); int code = 599; - Response response = mock(Response.class); - when(response.code()).thenReturn(code); + Response response = new TestResponse().code(code); double max = limiter.getLimit(); limiter.acquire(LimitEnforcement.DEFAULT_ENABLED).get().onSuccess(response); @@ -273,8 +327,7 @@ public void onSuccess_ignoresIfResponseIndicatesUnknownServerError_sticky() { public void onSuccess_dropsIfResponseIndicatesUnknownServerError_host() { CautiousIncreaseAggressiveDecreaseConcurrencyLimiter limiter = limiter(Behavior.HOST_LEVEL); int code = 599; - Response response = mock(Response.class); - when(response.code()).thenReturn(code); + Response response = new TestResponse().code(code); double max = limiter.getLimit(); limiter.acquire(LimitEnforcement.DEFAULT_ENABLED).get().onSuccess(response); diff --git a/dialogue-test-common/src/main/java/com/palantir/dialogue/TestResponse.java b/dialogue-test-common/src/main/java/com/palantir/dialogue/TestResponse.java index b9f7c4d29..9817625fb 100644 --- a/dialogue-test-common/src/main/java/com/palantir/dialogue/TestResponse.java +++ b/dialogue-test-common/src/main/java/com/palantir/dialogue/TestResponse.java @@ -19,7 +19,6 @@ import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ListMultimap; import com.google.common.net.HttpHeaders; -import com.google.errorprone.annotations.CheckReturnValue; import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.ByteArrayInputStream; @@ -58,7 +57,6 @@ public int code() { return code; } - @CheckReturnValue public TestResponse code(int value) { this.code = value; return this; @@ -93,12 +91,10 @@ private void checkNotClosed() { Preconditions.checkState(!isClosed(), "Please don't close twice"); } - @CheckReturnValue public TestResponse contentType(String contentType) { return withHeader(HttpHeaders.CONTENT_TYPE, contentType); } - @CheckReturnValue public TestResponse withHeader(String headerName, String headerValue) { this.headers = ImmutableListMultimap.builder() .putAll(headers) diff --git a/dialogue-test-common/src/main/java/com/palantir/dialogue/TestResponseQosEncoder.java b/dialogue-test-common/src/main/java/com/palantir/dialogue/TestResponseQosEncoder.java new file mode 100644 index 000000000..f00c4f272 --- /dev/null +++ b/dialogue-test-common/src/main/java/com/palantir/dialogue/TestResponseQosEncoder.java @@ -0,0 +1,28 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.dialogue; + +import com.palantir.conjure.java.api.errors.QosReasons; + +public enum TestResponseQosEncoder implements QosReasons.QosResponseEncodingAdapter { + INSTANCE; + + @Override + public void setHeader(TestResponse testResponse, String headerName, String headerValue) { + testResponse.withHeader(headerName, headerValue); + } +} From bc67cff449271d59c20199a59bb199874b31daed Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 8 Oct 2024 12:40:44 -0400 Subject: [PATCH 6/9] test PinUntilErrorNodeSelectionStrategy --- ...ErrorNodeSelectionStrategyChannelTest.java | 58 ++++++++++++++++++- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/dialogue-core/src/test/java/com/palantir/dialogue/core/PinUntilErrorNodeSelectionStrategyChannelTest.java b/dialogue-core/src/test/java/com/palantir/dialogue/core/PinUntilErrorNodeSelectionStrategyChannelTest.java index 0191ffee8..3520b6899 100644 --- a/dialogue-core/src/test/java/com/palantir/dialogue/core/PinUntilErrorNodeSelectionStrategyChannelTest.java +++ b/dialogue-core/src/test/java/com/palantir/dialogue/core/PinUntilErrorNodeSelectionStrategyChannelTest.java @@ -27,9 +27,14 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; +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.conjure.java.api.errors.QosReasons; import com.palantir.dialogue.Request; import com.palantir.dialogue.Response; import com.palantir.dialogue.TestResponse; +import com.palantir.dialogue.TestResponseQosEncoder; import com.palantir.dialogue.core.LimitedChannel.LimitEnforcement; import com.palantir.tritium.metrics.registry.DefaultTaggedMetricRegistry; import java.time.Duration; @@ -120,6 +125,51 @@ void http_429_responses_do_not_cause_node_switch() { .contains(429, 429, 429, 429, 429, 429); } + @Test + void http_503_responses_dueTo_custom_do_not_cause_node_switch() { + setResponse(channel1, 100); + setResponse(channel2, 204); + + assertThat(IntStream.range(0, 6).map(_number -> getCode(pinUntilErrorWithoutReshuffle))) + .describedAs("Should be locked on to channel2 initially") + .contains(204, 204, 204, 204, 204, 204); + + TestResponse response = new TestResponse().code(503); + QosReasons.encodeToResponse( + QosReason.builder().reason("reason").dueTo(DueTo.CUSTOM).build(), + response, + TestResponseQosEncoder.INSTANCE); + setResponse(channel2, response); + + assertThat(IntStream.range(0, 6).map(_number -> getCode(pinUntilErrorWithoutReshuffle))) + .describedAs("Even after receiving a 503 with DueTo.CUSTOM, we must stay pinned.") + .contains(503, 503, 503, 503, 503, 503); + } + + @Test + void http_503_responses_retryHint_noRetry_causes_node_switch() { + setResponse(channel1, 100); + setResponse(channel2, 204); + + assertThat(IntStream.range(0, 6).map(_number -> getCode(pinUntilErrorWithoutReshuffle))) + .describedAs("Should be locked on to channel2 initially") + .contains(204, 204, 204, 204, 204, 204); + + TestResponse response = new TestResponse().code(503); + QosReasons.encodeToResponse( + QosReason.builder() + .reason("reason") + .retryHint(RetryHint.DO_NOT_RETRY) + .build(), + response, + TestResponseQosEncoder.INSTANCE); + setResponse(channel2, response); + + assertThat(IntStream.range(0, 6).map(_number -> getCode(pinUntilErrorWithoutReshuffle))) + .describedAs("RetryHint.DO_NOT_RETRY has no impact on PinUntilError behavior.") + .contains(503, 100, 100, 100, 100, 100); + } + private void testStatusCausesNodeSwitch(int errorStatus) { before(); setResponse(channel1, 100); @@ -263,12 +313,16 @@ private static int getCode(PinUntilErrorNodeSelectionStrategyChannel channel, Re } private static void setResponse(LimitedChannel mockChannel, int status) { + Response resp = response(status); + setResponse(mockChannel, resp); + } + + private static void setResponse(LimitedChannel mockChannel, Response response) { Mockito.clearInvocations(mockChannel); Mockito.reset(mockChannel); - Response resp = response(status); lenient() .when(mockChannel.maybeExecute(any(), any(), eq(LimitEnforcement.DEFAULT_ENABLED))) - .thenReturn(Optional.of(Futures.immediateFuture(resp))); + .thenReturn(Optional.of(Futures.immediateFuture(response))); } private static Response response(int status) { From fe237ada2abdd3bdb5c6927f9627d3d9bc47174f Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 8 Oct 2024 12:58:07 -0400 Subject: [PATCH 7/9] Test RetryingChannel --- .../dialogue/core/RetryingChannelTest.java | 133 ++++++++++++++++-- 1 file changed, 123 insertions(+), 10 deletions(-) diff --git a/dialogue-core/src/test/java/com/palantir/dialogue/core/RetryingChannelTest.java b/dialogue-core/src/test/java/com/palantir/dialogue/core/RetryingChannelTest.java index c971f5848..a88865d04 100644 --- a/dialogue-core/src/test/java/com/palantir/dialogue/core/RetryingChannelTest.java +++ b/dialogue-core/src/test/java/com/palantir/dialogue/core/RetryingChannelTest.java @@ -28,6 +28,10 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; +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.conjure.java.api.errors.QosReasons; import com.palantir.conjure.java.client.config.ClientConfiguration; import com.palantir.dialogue.EndpointChannel; import com.palantir.dialogue.Request; @@ -35,6 +39,7 @@ import com.palantir.dialogue.Response; import com.palantir.dialogue.TestEndpoint; import com.palantir.dialogue.TestResponse; +import com.palantir.dialogue.TestResponseQosEncoder; import com.palantir.logsafe.exceptions.SafeIoException; import com.palantir.logsafe.exceptions.SafeRuntimeException; import java.io.ByteArrayOutputStream; @@ -135,9 +140,8 @@ public void testRetriesMax() { @Test public void retries_429s() throws Exception { - Response mockResponse = mock(Response.class); - when(mockResponse.code()).thenReturn(429); - when(channel.execute(any())).thenReturn(Futures.immediateFuture(mockResponse)); + when(channel.execute(any())).thenAnswer((Answer>) + _invocation -> Futures.immediateFuture(new TestResponse().code(429))); EndpointChannel retryer = new RetryingChannel( channel, @@ -149,17 +153,50 @@ public void retries_429s() throws Exception { ClientConfiguration.RetryOnTimeout.DISABLED); ListenableFuture response = retryer.execute(REQUEST); assertThat(response).isDone(); - assertThat(response.get()) + assertThat(response.get().code()) .as("After retries are exhausted the 429 response should be returned") - .isSameAs(mockResponse); + .isEqualTo(429); verify(channel, times(4)).execute(REQUEST); } @Test - public void retries_503s() throws Exception { - Response mockResponse = mock(Response.class); - when(mockResponse.code()).thenReturn(503); - when(channel.execute(any())).thenReturn(Futures.immediateFuture(mockResponse)); + public void retries_429s_dueTo_custom() throws Exception { + when(channel.execute(any())).thenAnswer((Answer>) _invocation -> { + TestResponse stubResponse = new TestResponse().code(429); + QosReasons.encodeToResponse( + QosReason.builder().reason("reason").dueTo(DueTo.CUSTOM).build(), + stubResponse, + TestResponseQosEncoder.INSTANCE); + return Futures.immediateFuture(stubResponse); + }); + + EndpointChannel retryer = new RetryingChannel( + channel, + TestEndpoint.POST, + "my-channel", + 3, + Duration.ZERO, + ClientConfiguration.ServerQoS.AUTOMATIC_RETRY, + ClientConfiguration.RetryOnTimeout.DISABLED); + ListenableFuture response = retryer.execute(REQUEST); + assertThat(response).isDone(); + assertThat(response.get().code()) + .as("After retries are exhausted the 429 response should be returned") + .isEqualTo(429); + verify(channel, times(4)).execute(REQUEST); + } + + @Test + public void does_not_retry_429_when_hinted() throws Exception { + TestResponse stubResponse = new TestResponse().code(429); + QosReasons.encodeToResponse( + QosReason.builder() + .reason("reason") + .retryHint(RetryHint.DO_NOT_RETRY) + .build(), + stubResponse, + TestResponseQosEncoder.INSTANCE); + when(channel.execute(any())).thenReturn(Futures.immediateFuture(stubResponse)); EndpointChannel retryer = new RetryingChannel( channel, @@ -172,11 +209,87 @@ public void retries_503s() throws Exception { ListenableFuture response = retryer.execute(REQUEST); assertThat(response).isDone(); assertThat(response.get()) + .as("The 429 response should be returned without retrying due to RetryHint.DO_NOT_RETRY") + .isSameAs(stubResponse); + verify(channel, times(1)).execute(REQUEST); + } + + @Test + public void retries_503s() throws Exception { + when(channel.execute(any())).thenAnswer((Answer>) + _invocation -> Futures.immediateFuture(new TestResponse().code(503))); + + EndpointChannel retryer = new RetryingChannel( + channel, + TestEndpoint.POST, + "my-channel", + 3, + Duration.ZERO, + ClientConfiguration.ServerQoS.AUTOMATIC_RETRY, + ClientConfiguration.RetryOnTimeout.DISABLED); + ListenableFuture response = retryer.execute(REQUEST); + assertThat(response).isDone(); + assertThat(response.get().code()) .as("After retries are exhausted the 503 response should be returned") - .isSameAs(mockResponse); + .isEqualTo(503); + verify(channel, times(4)).execute(REQUEST); + } + + @Test + public void retries_503s_dueTo_custom() throws Exception { + when(channel.execute(any())).thenAnswer((Answer>) _invocation -> { + TestResponse stubResponse = new TestResponse().code(503); + QosReasons.encodeToResponse( + QosReason.builder().reason("reason").dueTo(DueTo.CUSTOM).build(), + stubResponse, + TestResponseQosEncoder.INSTANCE); + return Futures.immediateFuture(stubResponse); + }); + + EndpointChannel retryer = new RetryingChannel( + channel, + TestEndpoint.POST, + "my-channel", + 3, + Duration.ZERO, + ClientConfiguration.ServerQoS.AUTOMATIC_RETRY, + ClientConfiguration.RetryOnTimeout.DISABLED); + ListenableFuture response = retryer.execute(REQUEST); + assertThat(response).isDone(); + assertThat(response.get().code()) + .as("After retries are exhausted the 503 response should be returned") + .isEqualTo(503); verify(channel, times(4)).execute(REQUEST); } + @Test + public void does_not_retry_503_when_hinted() throws Exception { + TestResponse stubResponse = new TestResponse().code(503); + QosReasons.encodeToResponse( + QosReason.builder() + .reason("reason") + .retryHint(RetryHint.DO_NOT_RETRY) + .build(), + stubResponse, + TestResponseQosEncoder.INSTANCE); + when(channel.execute(any())).thenReturn(Futures.immediateFuture(stubResponse)); + + EndpointChannel retryer = new RetryingChannel( + channel, + TestEndpoint.POST, + "my-channel", + 3, + Duration.ZERO, + ClientConfiguration.ServerQoS.AUTOMATIC_RETRY, + ClientConfiguration.RetryOnTimeout.DISABLED); + ListenableFuture response = retryer.execute(REQUEST); + assertThat(response).isDone(); + assertThat(response.get()) + .as("The 503 response should be returned without retrying due to RetryHint.DO_NOT_RETRY") + .isSameAs(stubResponse); + verify(channel, times(1)).execute(REQUEST); + } + @Test public void retries_308s() throws Exception { Response mockResponse = mock(Response.class); From 4a2709833e6f327945c8de38c9ee93080b8fc3f8 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 21 Oct 2024 13:06:02 -0400 Subject: [PATCH 8/9] fixup rebase --- ...sIncreaseAggressiveDecreaseConcurrencyLimiter.java | 11 ++--------- .../java/com/palantir/dialogue/core/Responses.java | 5 +++-- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/CautiousIncreaseAggressiveDecreaseConcurrencyLimiter.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/CautiousIncreaseAggressiveDecreaseConcurrencyLimiter.java index 02ecde58e..369989a2b 100644 --- a/dialogue-core/src/main/java/com/palantir/dialogue/core/CautiousIncreaseAggressiveDecreaseConcurrencyLimiter.java +++ b/dialogue-core/src/main/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; @@ -103,7 +101,7 @@ enum Behavior { void onSuccess(Response result, PermitControl control) { if (Responses.isTooManyRequests(result) || Responses.isInternalServerError(result) - || isQosDueToCustom(result)) { + || Responses.isQosDueToCustom(result)) { // 429, 500, or QoS due to a custom reason control.ignore(); } else if ((Responses.isQosStatus(result) && !Responses.isTooManyRequests(result)) @@ -127,7 +125,7 @@ void onFailure(Throwable throwable, PermitControl control) { ENDPOINT_LEVEL() { @Override void onSuccess(Response result, PermitControl control) { - if ((Responses.isTooManyRequests(result) && !isQosDueToCustom(result)) + if ((Responses.isTooManyRequests(result) && !Responses.isQosDueToCustom(result)) || Responses.isInternalServerError(result)) { // non-custom 429 or 500 control.dropped(); @@ -161,11 +159,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 { /** 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 index a49b4e2ab..f9dfc8b87 100644 --- a/dialogue-core/src/main/java/com/palantir/dialogue/core/Responses.java +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/Responses.java @@ -49,7 +49,7 @@ static boolean isQosDueToCustom(Response result) { return false; } QosReason reason = DialogueQosReasonDecoder.parse(result); - return DueTo.CUSTOM.equals(reason.dueTo().orElse(null)); + return reason.dueTo().isPresent() && DueTo.CUSTOM.equals(reason.dueTo().get()); } static boolean isRetryableQos(Response result) { @@ -57,7 +57,8 @@ static boolean isRetryableQos(Response result) { return false; } QosReason reason = DialogueQosReasonDecoder.parse(result); - return !RetryHint.DO_NOT_RETRY.equals(reason.retryHint().orElse(null)); + return reason.retryHint().isEmpty() + || !RetryHint.DO_NOT_RETRY.equals(reason.retryHint().get()); } static boolean isServerErrorRange(Response response) { From d7822db37d733b3bdf08a1c0f3ca5fdd39a32127 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Mon, 21 Oct 2024 17:06:21 +0000 Subject: [PATCH 9/9] Add generated changelog entries --- changelog/@unreleased/pr-2375.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-2375.v2.yml diff --git a/changelog/@unreleased/pr-2375.v2.yml b/changelog/@unreleased/pr-2375.v2.yml new file mode 100644 index 000000000..63b89408a --- /dev/null +++ b/changelog/@unreleased/pr-2375.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Handle new QoS Metadata + links: + - https://github.com/palantir/dialogue/pull/2375