Fix flaky retry tests #10887

larry-safran · 2024-02-01T21:15:46Z

Fix retry tests that became flaky. Fixes #10873

…ness

temawi · 2024-02-02T21:50:59Z

interop-testing/src/test/java/io/grpc/testing/integration/RetryTest.java

@@ -103,8 +105,11 @@ public class RetryTest {
  @Rule
  public final GrpcCleanupRule cleanupRule = new GrpcCleanupRule();
  private final FakeClock fakeClock = new FakeClock();
-  @Mock
-  private ClientCall.Listener<Integer> mockCallListener;
+  private TestListener realMockCallListener = new TestListener();


I wouldn't use the word "mock" since it's not a mock object. Just testCallListener would do I think, or people ofthen use the term "fake" with these kinds of classes.

changed to testCallListener

temawi · 2024-02-02T21:51:33Z

interop-testing/src/test/java/io/grpc/testing/integration/RetryTest.java

+  @Before
+  public void setUp() throws Exception {
+  }


This can be removed, right?

YifeiZhuang · 2024-02-02T22:13:06Z

core/src/main/java/io/grpc/internal/AbstractClientStream.java

        if (getTransportTracer() != null) {
          getTransportTracer().reportStreamClosed(status.isOk());
        }
+        listener().closed(status, rpcProgress, trailers);


You switched the order of the the listener and transportTracer.
The original tests is about verifying census stats(in statsTraceCtx), how does that related to transportTracer?

larry-safran · 2024-02-02T22:38:01Z

The test is checking the stats which get set in the tracer. It is waiting for the closed to be called to make the check, but when the closed method is called before the tracer report is generated there is a window where the report isn't available, which is what caused the failures.

…

On Fri, Feb 2, 2024 at 2:13 PM yifeizhuang ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/src/main/java/io/grpc/internal/AbstractClientStream.java <#10887 (comment)>: > if (getTransportTracer() != null) { getTransportTracer().reportStreamClosed(status.isOk()); } + listener().closed(status, rpcProgress, trailers); You switched the order of the the listener and transportTracer. The original tests is about verifying census stats(in statsTraceCtx), how does that related to transportTracer? — Reply to this email directly, view it on GitHub <#10887 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZQMCXWMGCWF454XV52NYNTYRVQH3AVCNFSM6AAAAABCVTLZR2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNRQGQ4DMMRYGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

YifeiZhuang · 2024-02-02T23:08:47Z

when the closed method is called before the tracer report is generated there is a window where the report isn't available, which is what caused the failures.

hmm, I had a theory about why the call closed method is called before those reports I put on the issue.

larry-safran · 2024-02-02T23:16:40Z

Your delay masked the window.

…

On Fri, Feb 2, 2024 at 3:08 PM yifeizhuang ***@***.***> wrote: when the closed method is called before the tracer report is generated there is a window where the report isn't available, which is what caused the failures. hmm, I had a theory about why the call closed method is called before those reports I put on the issue. — Reply to this email directly, view it on GitHub <#10887 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZQMCXRF3M27PCQYBO5R43TYRVWYVAVCNFSM6AAAAABCVTLZR2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRUHA4DIMRWGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

temawi

I don't have enough understanding of the original problem to know if @YifeiZhuang's concern here is warranted, but if she is ok with the change it LGTM.

YifeiZhuang · 2024-02-05T16:15:03Z

I agree that the test setup caused the flakiness, not a real problem. One thing to keep in mind: when canceling the retryable stream returns and close and master listener, all the substreams should have been closed already. Because notifying master listener is the last thing we should do in retryable stream. Otherwise there will be resource leak, e.g. RejectedExecutionException.
But I don't see the fix address that problem. In the current fix, the master listener may still happen before the substream listener is closed. It might mitigate the flakiness, but I'm not yet convinced this is the appropriate fix.
cc. @ejona86

* Reorder tracing and actually closing listener to eliminate test flakiness * Use real value rather than mock for flaky test

* Fix retries that timeout hanging forever. (#10855) Fixes #10336 * Fix flaky retry tests (#10887) * Reorder tracing and actually closing listener to eliminate test flakiness * Use real value rather than mock for flaky test

* Fix retries that timeout hanging forever. (#10855) Fixes #10336 * Fix flaky retry tests (#10887)

larry-safran added 4 commits February 1, 2024 13:11

improve debugging

d3438f3

Reorder tracing and actually closing listener to eliminate test flaki…

515df0c

…ness

Use real value rather than mock for flaky test

c8844f3

Use real value rather than mock for flaky test

991fb7d

larry-safran marked this pull request as ready for review February 2, 2024 21:38

larry-safran changed the title ~~improve debugging~~ Fix flaky retry tests Feb 2, 2024

larry-safran requested review from YifeiZhuang and temawi February 2, 2024 21:39

temawi reviewed Feb 2, 2024

View reviewed changes

Address review comments

19f5978

YifeiZhuang reviewed Feb 2, 2024

View reviewed changes

larry-safran mentioned this pull request Feb 2, 2024

interop-test, core: retryTest is flaky #10873

Closed

Change schedule delay for RetryTest no-op task to 1 ms instead of 0 ns.

e7cfe91

larry-safran requested review from YifeiZhuang and temawi February 2, 2024 23:09

Change schedule delay for RetryTest no-op task back to 0.

2d5fbc2

temawi approved these changes Feb 3, 2024

View reviewed changes

larry-safran merged commit 374dbe9 into grpc:master Feb 5, 2024
14 checks passed

larry-safran deleted the is_10873 branch February 5, 2024 18:55

larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Feb 5, 2024

Fix flaky retry tests (grpc#10887)

b0d0f72

* Reorder tracing and actually closing listener to eliminate test flakiness * Use real value rather than mock for flaky test

larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Feb 5, 2024

Fix flaky retry tests (grpc#10887)

659eed6

* Reorder tracing and actually closing listener to eliminate test flakiness * Use real value rather than mock for flaky test

larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Feb 5, 2024

Fix flaky retry tests (grpc#10887)

988b5fa

* Reorder tracing and actually closing listener to eliminate test flakiness * Use real value rather than mock for flaky test

larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Feb 5, 2024

Fix flaky retry tests (grpc#10887)

fd0aae8

* Reorder tracing and actually closing listener to eliminate test flakiness * Use real value rather than mock for flaky test

larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Feb 5, 2024

Fix flaky retry tests (grpc#10887)

5c58179

* Reorder tracing and actually closing listener to eliminate test flakiness * Use real value rather than mock for flaky test

larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Feb 5, 2024

Fix flaky retry tests (grpc#10887)

9de490f

* Reorder tracing and actually closing listener to eliminate test flakiness * Use real value rather than mock for flaky test

larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Feb 5, 2024

Fix flaky retry tests (grpc#10887)

340e493

* Reorder tracing and actually closing listener to eliminate test flakiness * Use real value rather than mock for flaky test

larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Feb 5, 2024

Fix flaky retry tests (grpc#10887)

3849df5

* Reorder tracing and actually closing listener to eliminate test flakiness * Use real value rather than mock for flaky test

larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Feb 5, 2024

Fix flaky retry tests (grpc#10887)

a7a3947

* Reorder tracing and actually closing listener to eliminate test flakiness * Use real value rather than mock for flaky test

larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Feb 5, 2024

Fix flaky retry tests (grpc#10887)

87c8ee3

* Reorder tracing and actually closing listener to eliminate test flakiness * Use real value rather than mock for flaky test

larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Feb 5, 2024

Fix flaky retry tests (grpc#10887)

a2386e1

* Reorder tracing and actually closing listener to eliminate test flakiness * Use real value rather than mock for flaky test

larry-safran added a commit that referenced this pull request Feb 6, 2024

Fix retries that timeout hanging forever. [ Backport to 1.60.x] (#10885)

b8cd82c

* Fix retries that timeout hanging forever. (#10855) Fixes #10336 * Fix flaky retry tests (#10887)

github-actions bot locked as resolved and limited conversation to collaborators May 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix flaky retry tests #10887

Fix flaky retry tests #10887

larry-safran commented Feb 1, 2024 •

edited

Loading

temawi Feb 2, 2024

larry-safran Feb 2, 2024

temawi Feb 2, 2024

larry-safran Feb 2, 2024

YifeiZhuang Feb 2, 2024

larry-safran commented Feb 2, 2024 via email

YifeiZhuang commented Feb 2, 2024

larry-safran commented Feb 2, 2024 via email

temawi left a comment

YifeiZhuang commented Feb 5, 2024

Fix flaky retry tests #10887

Fix flaky retry tests #10887

Conversation

larry-safran commented Feb 1, 2024 • edited Loading

temawi Feb 2, 2024

Choose a reason for hiding this comment

larry-safran Feb 2, 2024

Choose a reason for hiding this comment

temawi Feb 2, 2024

Choose a reason for hiding this comment

larry-safran Feb 2, 2024

Choose a reason for hiding this comment

YifeiZhuang Feb 2, 2024

Choose a reason for hiding this comment

larry-safran commented Feb 2, 2024 via email

YifeiZhuang commented Feb 2, 2024

larry-safran commented Feb 2, 2024 via email

temawi left a comment

Choose a reason for hiding this comment

YifeiZhuang commented Feb 5, 2024

larry-safran commented Feb 1, 2024 •

edited

Loading