A50: Exclude locally-initiated client cancellations from Call Counter (#12923) - #558
Open
AgraVator wants to merge 5 commits into
Open
A50: Exclude locally-initiated client cancellations from Call Counter (#12923)#558AgraVator wants to merge 5 commits into
AgraVator wants to merge 5 commits into
Conversation
kannanjgithub
approved these changes
Aug 4, 2026
ejona86
reviewed
Aug 7, 2026
| When the child policy asks for a subchannel, the `outlier_detection` will wrap the subchannel with a wrapper (see [Subchannel Wrapper section](#subchannel-wrapper)). Then, the subchannel wrapper will be added to the list in the map entry for its address, if that map entry exists. If there is no map entry, or if the subchannel is created with multiple addresses, the subchannel will be ignored for outlier detection. If that address is currently ejected, that subchannel wrapper's `eject` method will be called. | ||
|
|
||
| The `outlier_detection` LB policy will provide a picker that delegates to the child policy's picker, and when the request finishes, increment the corresponding counter in the map entry referenced by the subchannel wrapper that was picked. If both the `success_rate_ejection` and `failure_percentage_ejection` fields are unset in the configuration, the picker should not do that counting. | ||
| The `outlier_detection` LB policy will provide a picker that delegates to the child policy's picker, and when the request finishes, increment the corresponding counter in the map entry referenced by the subchannel wrapper that was picked. Locally-initiated client-side cancellations (hedging cancellations, application `CANCELLED`, and `DEADLINE_EXCEEDED`) MUST be excluded from `successCount` and `failureCount`. If both the `success_rate_ejection` and `failure_percentage_ejection` fields are unset in the configuration, the picker should not do that counting. |
Member
There was a problem hiding this comment.
application CANCELLED and DEADLINE_EXCEEDED shouldn't be in all-upper-case, as it makes it seem like this is based on the status code. Just describe them. ("application cancellation, and deadline exceeded")
| When the child policy asks for a subchannel, the `outlier_detection` will wrap the subchannel with a wrapper (see [Subchannel Wrapper section](#subchannel-wrapper)). Then, the subchannel wrapper will be added to the list in the map entry for its address, if that map entry exists. If there is no map entry, or if the subchannel is created with multiple addresses, the subchannel will be ignored for outlier detection. If that address is currently ejected, that subchannel wrapper's `eject` method will be called. | ||
|
|
||
| The `outlier_detection` LB policy will provide a picker that delegates to the child policy's picker, and when the request finishes, increment the corresponding counter in the map entry referenced by the subchannel wrapper that was picked. If both the `success_rate_ejection` and `failure_percentage_ejection` fields are unset in the configuration, the picker should not do that counting. | ||
| The `outlier_detection` LB policy will provide a picker that delegates to the child policy's picker, and when the request finishes, increment the corresponding counter in the map entry referenced by the subchannel wrapper that was picked. Locally-initiated client-side cancellations (hedging cancellations, application `CANCELLED`, and `DEADLINE_EXCEEDED`) MUST be excluded from `successCount` and `failureCount`. If both the `success_rate_ejection` and `failure_percentage_ejection` fields are unset in the configuration, the picker should not do that counting. |
Member
There was a problem hiding this comment.
Don't do MUST. This document is not using that style.
| When the child policy asks for a subchannel, the `outlier_detection` will wrap the subchannel with a wrapper (see [Subchannel Wrapper section](#subchannel-wrapper)). Then, the subchannel wrapper will be added to the list in the map entry for its address, if that map entry exists. If there is no map entry, or if the subchannel is created with multiple addresses, the subchannel will be ignored for outlier detection. If that address is currently ejected, that subchannel wrapper's `eject` method will be called. | ||
|
|
||
| The `outlier_detection` LB policy will provide a picker that delegates to the child policy's picker, and when the request finishes, increment the corresponding counter in the map entry referenced by the subchannel wrapper that was picked. If both the `success_rate_ejection` and `failure_percentage_ejection` fields are unset in the configuration, the picker should not do that counting. | ||
| The `outlier_detection` LB policy will provide a picker that delegates to the child policy's picker, and when the request finishes, increment the corresponding counter in the map entry referenced by the subchannel wrapper that was picked. Locally-initiated client-side cancellations (hedging cancellations, application `CANCELLED`, and `DEADLINE_EXCEEDED`) MUST be excluded from `successCount` and `failureCount`. If both the `success_rate_ejection` and `failure_percentage_ejection` fields are unset in the configuration, the picker should not do that counting. |
Member
There was a problem hiding this comment.
Referencing successCount and failureCount before they are defined is confusing.
I think we could avoid the change in this paragraph. The Call Counter section is the one that looks to be really defining this. Maybe we can make this section more generic so it won't be assumed to be thorough by s/increment the corresponding counter/updates the counter/. Michael may have a preference.
At that point I think we remove defining successCount and failureCount as variables, and remove the reference to the variable names in the rationale.
|
|
||
| ### Excluding Locally-Initiated Client-Side Cancellations | ||
|
|
||
| Locally-initiated client-side cancellations (hedging cancellations, application `CANCELLED`, and `DEADLINE_EXCEEDED`) are excluded from outlier detection counting (`successCount` and `failureCount`). For locally-initiated cancellations (hedging cancellations of non-winning sibling attempts and application-initiated cancellations), this aligns with Envoy's `resetStream()` behavior and prevents false-positive ejections during hedging. For `DEADLINE_EXCEEDED`, this differs from Envoy (which counts deadline expirations as failures for outlier detection); however, because deadline expiration semantics vary between Envoy and gRPC, and because a deadline expiration cannot be definitively attributed to a server failure rather than a network delay, gRPC treats it as a client-side cancellation. |
Member
There was a problem hiding this comment.
"semantics vary" is pretty vague. "gRPC can't reliably distinguish between cancellation and deadline exceeded cross-language" seems relevant.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Clarifies gRFC A50 (
A50-xds-outlier-detection.md) to explicitly define that locally-initiated client-side cancellations MUST be excluded from the Outlier Detection Call Counter (successCountandfailureCount), following the cross-language discussion consensus.Key Clarifications
hedging cancellations, applicationCANCELLED, andDEADLINE_EXCEEDED) from counter increments on call completion.successCountandfailureCount, omitting locally cancelled calls.DEADLINE_EXCEEDED, which is treated as a client-side cancellation from first principles and for implementation simplicity) are excluded from outlier detection counting to align with Envoy'sresetStream()behavior and prevent false-positive ejections during hedging.Related Work