Handle new QoS Metadata #2375
Handle new QoS Metadata #2375
Conversation
Generate changelog in
|
carterkozak
force-pushed
the
ckozak/qos-metadata
branch
from
8db0658
to
fe237ad
Compare
| @@ -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) | |||
There was a problem hiding this comment.
if i understand this correctly, a Due-To=CUSTOM will not count as an error for node pinning, and we will stay on the same node? What's the rationale behind that?
There was a problem hiding this comment.
Due-to custom means the client shouldn’t attempt to interpret meaning from the failure, so we don’t route to another node in that case. Changing pinned nodes can have dramatic impacts on perf, some clients use it for cache locality, which we don’t want to give up unless we really need to (instability/etc)
| import com.palantir.dialogue.Response; | ||
| import java.util.Optional; | ||
|
|
||
| enum DialogueQosReasonDecoder implements QosResponseDecodingAdapter<Response> { |
There was a problem hiding this comment.
any reason this could not live along side Response in dialogue-target and be shared across projects?
There was a problem hiding this comment.
It’s an implementation detail, not part of the codegen target api. I don’t mind duplication for such a simple utility, primary goal is to avoid expanding the public api unnecessarily
|
👍 lgtm |
|
Released 4.2.0 |
See palantir/conjure-java-runtime-api#1231 for more information
After this PR
==COMMIT_MSG==
Handle new QoS Metadata
==COMMIT_MSG==