-
Notifications
You must be signed in to change notification settings - Fork 4k
core: exclude client and hedging cancellations from callcounters for outlier-detection #12923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 15 commits
9d14356
7abb485
f9cc87f
120e35c
f9dd0fb
fe581ae
5e558a5
634be81
20682d9
bf90eb5
6d3cf19
9331806
5f3af3d
be50f7b
c6e2f4a
f27ce70
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -197,6 +197,9 @@ public final void halfClose() { | |
| @Override | ||
| public final void cancel(Status reason) { | ||
| Preconditions.checkArgument(!reason.isOk(), "Should not cancel with OK status"); | ||
| if (cancelled || transportState().isListenerClosed()) { | ||
|
AgraVator marked this conversation as resolved.
Outdated
|
||
| return; | ||
| } | ||
| cancelled = true; | ||
| abstractClientStreamSink().cancel(reason); | ||
| } | ||
|
|
@@ -251,6 +254,10 @@ protected TransportState( | |
| } | ||
| } | ||
|
|
||
| protected final boolean isListenerClosed() { | ||
|
AgraVator marked this conversation as resolved.
Outdated
|
||
| return listenerClosed; | ||
| } | ||
|
|
||
| private void setFullStreamDecompression(boolean fullStreamDecompression) { | ||
| this.fullStreamDecompression = fullStreamDecompression; | ||
| } | ||
|
|
@@ -435,13 +442,13 @@ public final void transportReportStatus( | |
|
|
||
| if (deframerClosed) { | ||
| deframerClosedTask = null; | ||
| closeListener(status, rpcProgress, trailers); | ||
| closeListener(status, rpcProgress, trailers, stopDelivery); | ||
| } else { | ||
| deframerClosedTask = | ||
| new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| closeListener(status, rpcProgress, trailers); | ||
| closeListener(status, rpcProgress, trailers, stopDelivery); | ||
| } | ||
| }; | ||
| closeDeframer(stopDelivery); | ||
|
|
@@ -454,9 +461,12 @@ public void run() { | |
| * @throws IllegalStateException if the call has not yet been started. | ||
| */ | ||
| private void closeListener( | ||
| Status status, RpcProgress rpcProgress, Metadata trailers) { | ||
| Status status, RpcProgress rpcProgress, Metadata trailers, boolean stopDelivery) { | ||
|
AgraVator marked this conversation as resolved.
Outdated
|
||
| if (!listenerClosed) { | ||
| listenerClosed = true; | ||
| if (stopDelivery) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using
(3) is a problem. In Netty transport for example, NettyClientHandler.forcefulClose explicitly sets Change this to:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, we definitely shouldn't be doing any logic here based on the status code. What is wrong with "Transport Forceful Shutdown'? That is defined as cancelling all streams, so it should behave the same as call.cancel(). I can believe we may need to change some cases to stop using
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to count transport forceful shutdown in response to network drop as failure because it is not client app initiated although it might be client network stack initiated. Envoy does count connection drops as failures (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed However, we should consider the following cases in addition to the one Kannan pointed out:
ProposalShould we decouple these two responsibilities in public final void transportReportStatus(
Status status, RpcProgress rpcProgress, boolean stopDelivery, boolean isClientCancelled, Metadata trailers)This allows transport call sites to specify Or Alternatively, we can go and change the stopDelivery values for these cases (although it will result in behaviour changes)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can ignore the case I mentioned NettyClientHandler.forcefulClose - it is only called during channel shutdown by the application.
These should be counted as failures.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noted. |
||
| statsTraceCtx.clientCancelled(status); | ||
| } | ||
| statsTraceCtx.streamClosed(status); | ||
| if (getTransportTracer() != null) { | ||
| getTransportTracer().reportStreamClosed(status.isOk()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| /* | ||
| * Copyright 2026 The gRPC Authors | ||
| * | ||
| * 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 io.grpc.internal; | ||
|
|
||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.verify; | ||
| import static org.mockito.Mockito.verifyNoInteractions; | ||
|
|
||
| import io.grpc.ClientStreamTracer; | ||
| import io.grpc.ServerStreamTracer; | ||
| import io.grpc.Status; | ||
| import io.grpc.StreamTracer; | ||
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; | ||
| import org.junit.runners.JUnit4; | ||
|
|
||
| /** Unit tests for {@link StatsTraceContext}. */ | ||
| @RunWith(JUnit4.class) | ||
| public class StatsTraceContextTest { | ||
|
|
||
| @Test | ||
| public void clientCancelled_notifiesClientStreamTracers() { | ||
| ClientStreamTracer clientTracer = mock(ClientStreamTracer.class); | ||
| ServerStreamTracer serverTracer = mock(ServerStreamTracer.class); | ||
|
|
||
| StatsTraceContext statsTraceCtx = new StatsTraceContext( | ||
| new StreamTracer[] {clientTracer, serverTracer}); | ||
|
|
||
| Status cancelledStatus = Status.CANCELLED.withDescription("Client cancelled"); | ||
| statsTraceCtx.clientCancelled(cancelledStatus); | ||
|
|
||
| verify(clientTracer).cancelled(cancelledStatus); | ||
| verifyNoInteractions(serverTracer); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.