chore: Shutdown and awaitTermination for showcase clients #1669

lqiu96 · 2023-05-09T14:11:43Z

Seeing error messages regarding orphaned channels and not cleaning up the queues for gRPC.
Error:

io.grpc.internal.ManagedChannelOrphanWrapper$ManagedChannelReference cleanQueue
SEVERE: *~*~*~ Previous channel ManagedChannelImpl{logId=23, target=localhost:7469} was not shutdown properly!!! ~*~*~*
    Make sure to call shutdown()/shutdownNow() and wait until awaitTermination() returns true.

Few changes in this PR:

Create a single client to be used for each file (no longer for each test case).
Run the tests sequentially to not overwhelm the showcase server
await for the client to terminate (wait 10 seconds)

Fixes: #1680

lqiu96 · 2023-05-09T19:08:00Z

httpjsonClient.awaitTermination() does nothing as of now.

blakeli0 · 2023-05-09T22:19:17Z

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITCommonServiceMixins.java

+    grpcClient.awaitTermination(TestClientInitializer.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS);
+
+    httpjsonClient.close();
+    httpjsonClient.awaitTermination(


This method is not implemented yet #1663

blakeli0

Do we know why the deadline PR caused flakyness? The deadline PR should only affect http/json stuff in theory.

lqiu96 · 2023-05-09T22:58:01Z

Do we know why the deadline PR caused flakyness? The deadline PR should only affect http/json stuff in theory.

I believe it's just the addition of a bunch of tests added for the deadline PR. These tests run in parallel and the showcase server was never built for performance or for handling being spammed with retries. I added a bit of wait for the tests and have the tests run sequentially to try and help with the CI jobs.

lqiu96 · 2023-05-12T18:16:05Z

gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/ManagedHttpJsonChannel.java

-  private static final ExecutorService DEFAULT_EXECUTOR =
-      InstantiatingExecutorProvider.newBuilder().build().getExecutor();


Static DEFAULT_EXECUTOR error'd out on the unit tests. Same executor was being used and it was shutdown after the first test.

lqiu96 · 2023-05-12T18:16:45Z

showcase/pom.xml

-            <configuration>
-              <forkCount>1C</forkCount>
-              <reuseForks>true</reuseForks>
-            </configuration>


Run the tests sequentially for now to see if it helps with the CI jobs.

Can we revert this change with the fix?

lqiu96 · 2023-05-12T18:19:40Z

I'll try to break this PR into two PRs (one for awaitTermination() implementation and another into showcase fixes). I tried to group them together since the new PR might fail the due to the flaky CI jobs.

lqiu96 · 2023-05-12T19:18:21Z

Separate PR created here: #1677

sonarcloud · 2023-06-05T16:05:18Z

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

sonarcloud · 2023-06-05T16:10:06Z

[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

sonarcloud · 2023-06-05T16:14:56Z

[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

blakeli0 · 2023-06-05T19:28:50Z

Few changes in this PR:

Create a single client to be used for each file (no longer for each test case).

Run the tests sequentially to not overwhelm the showcase server

await for the client to terminate (wait 10 seconds)

1 and 3 sounds good to me, with both 1 and 3 in, I think we may not need 2 anymore.

lqiu96 · 2023-06-05T20:06:53Z

Few changes in this PR:

Create a single client to be used for each file (no longer for each test case).

Run the tests sequentially to not overwhelm the showcase server

await for the client to terminate (wait 10 seconds)

1 and 3 sounds good to me, with both 1 and 3 in, I think we may not need 2 anymore.

I kept 2 in to help the server with any flakes (especially for the deadline tests that spam the server). The overall runtime increases from 45 seconds to ~2 minutes which I don't think is too bad. @blakeli0 Would you be fine with keeping this in for a few weeks to try and observe some CI stability. If this helps mitigate the issues, I can change it to be parallel again?

blakeli0 · 2023-06-05T20:46:28Z

Few changes in this PR:

Create a single client to be used for each file (no longer for each test case).

Run the tests sequentially to not overwhelm the showcase server

await for the client to terminate (wait 10 seconds)

1 and 3 sounds good to me, with both 1 and 3 in, I think we may not need 2 anymore.

I kept 2 in to help the server with any flakes (especially for the deadline tests that spam the server). The overall runtime increases from 45 seconds to ~2 minutes which I don't think is too bad. @blakeli0 Would you be fine with keeping this in for a few weeks to try and observe some CI stability. If this helps mitigate the issues, I can change it to be parallel again?

SGTM, with more showcase tests being added, we should revisit this soon though. CC: @burkedavison in case there is any concerns.

blakeli0

LGTM if we can get a green light from Burke on changing the test to sequential.

burkedavison · 2023-06-06T14:41:28Z

SGTM. Could we create a "reminder" issue to revisit the parallel / sequential decision in a few weeks?

lqiu96 · 2023-06-06T14:57:05Z

SGTM. Could we create a "reminder" issue to revisit the parallel / sequential decision in a few weeks?

Yep. I will revisit this July 6th.

chore: Properly shutdown and awaitTermination for showcase clients

28812e5

lqiu96 requested a review from blakeli0 May 9, 2023 14:11

lqiu96 requested a review from a team as a code owner May 9, 2023 14:11

product-auto-label bot added the size: m Pull request size is medium. label May 9, 2023

lqiu96 marked this pull request as draft May 9, 2023 14:36

lqiu96 added 2 commits May 9, 2023 12:11

chore: Update showcase tests

3df7edd

chore: Increase the initial unary timeout value

8569bf4

lqiu96 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 9, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 9, 2023

chore: Await Termination after close()

b79275f

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels May 9, 2023

chore: Run showcase tests one by one

65eb06e

lqiu96 marked this pull request as ready for review May 9, 2023 21:52

lqiu96 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 9, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 9, 2023

blakeli0 reviewed May 9, 2023

View reviewed changes

chore: Update to use the constant values

b2d5769

lqiu96 added 9 commits May 11, 2023 16:19

fix: Implement awaitTermination for ManagedHttpJsonChannel

20f3083

Merge branch 'main-await_termination' into main-shutdown_showcase_tests

f7c1543

fix: ManagedHttpJsonChannel's executor is not static

74f8960

chore: Update ManagedHttpJsonChannel logic

6a8f3ba

chore: Update LRO retry values

baf4679

chore: Create and Destroy clients once per test

e4fd3a3

chore: Resolve merge conflicts

ca17198

chore: Address code smell

3c82498

chore: Update LRO retry values

0141c23

lqiu96 added 4 commits May 12, 2023 12:05

chore: Use TTL for LROs

0acb243

chore: Use LRO defaults for initial callable

f8dc9ac

chore: Run showcase tests one by one

ced4197

chore: Empty commit

3a4e1b8

lqiu96 commented May 12, 2023

View reviewed changes

burkedavison mentioned this pull request May 15, 2023

showcase: Flaky ITLongRunningOperation.testGRPC_LROUnsuccessfulResponse_exceedsTotalTimeout_throwsDeadlineExceededException #1680

Closed

lqiu96 added 2 commits June 2, 2023 17:45

Merge branch 'main' into main-shutdown_showcase_tests

fa3b8db

Merge branch 'main' into main-shutdown_showcase_tests

d39bee7

lqiu96 added the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 2, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 2, 2023

lqiu96 mentioned this pull request Jun 5, 2023

showcase: sonarcloud build on main fails with errors from cleanQueue for ManagedChannel #1744

Closed

blakeli0 approved these changes Jun 5, 2023

View reviewed changes

lqiu96 requested a review from burkedavison June 5, 2023 20:55

burkedavison approved these changes Jun 6, 2023

View reviewed changes

lqiu96 merged commit e78b267 into main Jun 6, 2023
18 checks passed

lqiu96 deleted the main-shutdown_showcase_tests branch June 6, 2023 14:57

lqiu96 mentioned this pull request Jul 5, 2023

chore: Re-enable parallel showcase tests #1828

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Shutdown and awaitTermination for showcase clients #1669

chore: Shutdown and awaitTermination for showcase clients #1669

lqiu96 commented May 9, 2023 •

edited

lqiu96 commented May 9, 2023

blakeli0 May 9, 2023

blakeli0 left a comment

lqiu96 commented May 9, 2023 •

edited

lqiu96 May 12, 2023

lqiu96 May 12, 2023

blakeli0 Jun 5, 2023

lqiu96 commented May 12, 2023

lqiu96 commented May 12, 2023

sonarcloud bot commented Jun 5, 2023

sonarcloud bot commented Jun 5, 2023

sonarcloud bot commented Jun 5, 2023

blakeli0 commented Jun 5, 2023

lqiu96 commented Jun 5, 2023

blakeli0 commented Jun 5, 2023

blakeli0 left a comment

burkedavison commented Jun 6, 2023

lqiu96 commented Jun 6, 2023

		private static final ExecutorService DEFAULT_EXECUTOR =
		InstantiatingExecutorProvider.newBuilder().build().getExecutor();

chore: Shutdown and awaitTermination for showcase clients #1669

chore: Shutdown and awaitTermination for showcase clients #1669

Conversation

lqiu96 commented May 9, 2023 • edited

lqiu96 commented May 9, 2023

blakeli0 May 9, 2023

Choose a reason for hiding this comment

blakeli0 left a comment

Choose a reason for hiding this comment

lqiu96 commented May 9, 2023 • edited

lqiu96 May 12, 2023

Choose a reason for hiding this comment

lqiu96 May 12, 2023

Choose a reason for hiding this comment

blakeli0 Jun 5, 2023

Choose a reason for hiding this comment

lqiu96 commented May 12, 2023

lqiu96 commented May 12, 2023

sonarcloud bot commented Jun 5, 2023

sonarcloud bot commented Jun 5, 2023

sonarcloud bot commented Jun 5, 2023

blakeli0 commented Jun 5, 2023

lqiu96 commented Jun 5, 2023

blakeli0 commented Jun 5, 2023

blakeli0 left a comment

Choose a reason for hiding this comment

burkedavison commented Jun 6, 2023

lqiu96 commented Jun 6, 2023

lqiu96 commented May 9, 2023 •

edited

lqiu96 commented May 9, 2023 •

edited