fix: stop overriding default grpc executor #1355

mutianf · 2021-04-28T21:24:21Z

By default, gRPC uses a CachedThreadExecutor, and doesn't limit the number of threads. However in gax, TransportChannelProvider executor is shared with background executors (for retry, batching etc), and the number of threads is limited. This causes performance issues in the client library.

Currently, there are 2 ways to override TransportChannelProvider's executor:

If TransportChannelProvider doesn't have an executor, caller could set it by calling settings.stubSettings().setExecutor(executor)
Set a TransportChannelProvider with an executor:

settings.stubSettings().
  setTransportChannelProvider(
      settings.stubSettings().getTransportChanneProvider()
         .withExecutor(transportExecutor)
        .build())

The previous pr that tries to fix this issue (#869) may break case 1 when callers are setting transport channel executor from stubSettings#setExecutor(). This pr is another attempt to fix the issue and make it backward compatible.

The main changes are:

Add a backgroundExecutorProvider in StubSettings, and use a boolean to track if ClientSettings#setExecutorProvider() is called.
Add InstantiatingExecutorProvider as the default executor for ManagedHttpJsonChannel , So setting executorProvider to null won't break InstantiatingHttpJsonChannelProvider.

Moving forward, after stubSettings#setExecutorProvider() is deprecated, the only way to set the TransportChannelProvider's executor is transportChannelProvider#withExecutor().

codecov · 2021-04-28T21:26:29Z

Codecov Report

Merging #1355 (d076e43) into master (52e39f8) will increase coverage by 0.63%.
The diff coverage is 89.74%.

❗ Current head d076e43 differs from pull request most recent head 7a017cc. Consider uploading reports for the commit 7a017cc to get more accurate results

@@             Coverage Diff              @@
##             master    #1355      +/-   ##
============================================
+ Coverage     81.46%   82.10%   +0.63%     
- Complexity     1347     1356       +9     
============================================
  Files           211      211              
  Lines          5730     5705      -25     
  Branches        525      502      -23     
============================================
+ Hits           4668     4684      +16     
+ Misses          850      806      -44     
- Partials        212      215       +3

Impacted Files	Coverage Δ
...api/gax/grpc/InstantiatingGrpcChannelProvider.java	`78.19% <0.00%> (+0.73%)`	⬆️
...httpjson/InstantiatingHttpJsonChannelProvider.java	`75.00% <0.00%> (+5.15%)`	⬆️
...main/java/com/google/api/gax/rpc/StubSettings.java	`75.47% <83.33%> (-0.21%)`	⬇️
...oogle/api/gax/httpjson/ManagedHttpJsonChannel.java	`87.93% <100.00%> (+33.93%)`	⬆️
...ain/java/com/google/api/gax/rpc/ClientContext.java	`92.04% <100.00%> (+0.18%)`	⬆️
...in/java/com/google/api/gax/rpc/ClientSettings.java	`95.06% <100.00%> (+1.31%)`	⬆️
.../google/api/gax/batching/NonBlockingSemaphore.java	`76.31% <0.00%> (-5.27%)`	⬇️
...n/java/com/google/api/gax/grpc/GrpcStatusCode.java	`90.90% <0.00%> (-4.55%)`	⬇️
.../java/com/google/api/gax/batching/BatcherImpl.java	`94.28% <0.00%> (-1.79%)`	⬇️
...m/google/api/gax/batching/FlowControlSettings.java	`76.92% <0.00%> (-1.65%)`	⬇️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25a23db...7a017cc. Read the comment docs.

gax/src/main/java/com/google/api/gax/rpc/ClientContext.java

gax/src/main/java/com/google/api/gax/rpc/ClientSettings.java

gax/src/main/java/com/google/api/gax/rpc/StubSettings.java

igorbernstein2

lgtm

igorbernstein2 · 2021-05-05T15:33:14Z

@vam-google ping

vam-google

Overall the change is less scary than I thought it was, which is great.

Few more things, not mentioned in the individual comments:

Please update PR description to match the current state of the PR (looks like there were renaming done as PR feedback at least (workerExecutorProvider became backgroundExecutorProvider etc).
It is still not clear for me why the ultimate goal is to disable setting executor in the settings (i.e. stubSettings#setExecutorProvider()) and transportChannelProvider#withExecutor() must become the only way. Why can't we still keep both ways? I.e. it seems like relying on grpc default executor does not necessarily require deprecating the current gax's way of setting custom executor provider, if somebody wants to do so.

vam-google · 2021-06-03T23:02:06Z

gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java

+    try {
+      Channel channel = transportChannel.getChannel();
+
+      ClientCall<com.google.type.Color, Money> call =


com.google.type.Color is in the imports list of this file, fully qualified name is redundant here.

vam-google · 2021-06-03T23:09:28Z

gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java

+            .build();
+
+    // The default name thread name for grpc threads configured in GrpcUtil
+    assertThat(extractExecutorThreadName(provider)).contains("grpc-default-executor");


This makes this test depend on an undocumented private name of the thread: https://github.com/grpc/grpc-java/blob/v1.37.x/core/src/main/java/io/grpc/internal/GrpcUtil.java#L529. We can't depend on stuff like that, as it may change and in general it is way too implementation-specific.

It seems what this test actually is trying to check if the executor hasn't been overridden, for that, instead of explicitly validating that the executor matches some private stuff in gRPC, it should be enough to check that the executor is not the one provided by gax (i.e. instead of checking that the executor == internal gRPC stuff it should be enough to check that it is != gax stuff).

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java

vam-google · 2021-06-03T23:16:48Z

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

 import java.util.concurrent.TimeUnit;
 import javax.annotation.Nullable;

 /** Implementation of HttpJsonChannel which can issue http-json calls. */
 @BetaApi
 public class ManagedHttpJsonChannel implements HttpJsonChannel, BackgroundResource {
  private static final JsonFactory JSON_FACTORY = GsonFactory.getDefaultInstance();
+  private static final ExecutorService DEFAULT_EXECUTOR =


Can we do the same thing for gRPC InstantiatingChannelProvider? I.e. would Executors.newCachedThreadPool() instantiated by grpc be any different from Executors.newCachedThreadPool() instantiated in gax?

Also, I'm not sure that we need to touch HTTP stuff here at all, as it is not necessarily good to repeat in http world everything which is done in grpc.

We could do something similar for grpc channel provider. I thought it might be better to use the default grpc one in case grpc update it to something more efficient in the future. And also since grpc is already providing default executor, we don't need to repeat it in gax?

I'm setting a default http executor here because I'm deprecating needsExecutor, and http channel provider doesn't have a default one. If we're not sure about setting it to Executors.newCachedThreadPool(), maybe we can use the default gax executor here? And I think it's better to separate channel executor and background executor?

vam-google · 2021-06-03T23:19:35Z

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

@@ -147,7 +158,11 @@ public static Builder newBuilder() {
    private Builder() {}

    public Builder setExecutor(Executor executor) {
-      this.executor = executor;
+      if (executor != null) {


This is potentially very confusing and error-prone. If someone wants to set this thing to null, especially while we are still just in the builder, it should probably be allowed. Even in current implementation, if we don't call setExecutor() at all on the builder, the executor will remain null, but if we call setExecutor(nlll) it will magically become DEFAULT_EXECUTOR which is inconsistent and very confusing behavior.

Instead of doing this, please consider either failing on build() (line 184) method implementation if executor is still null in the builder when build() function is called, or subsituting null with default DEFAULT_EXECUTOR (i.e. effectively do the same thing, what you do here, but in the build() method instead of the setExecutor() method).

vam-google · 2021-06-03T23:49:32Z

gax/src/main/java/com/google/api/gax/rpc/ClientContext.java

+    if (executorProvider != null
+        && executor != null
+        && executorProvider.shouldAutoClose()
+        && executor != backgroundExecutor) {


How this specific statement executor != backgroundExecutor can be false? Given the code above, it seems that they may be equal only if both are equal to null, but that is already tested in this if statement above.

This is just to catch the case where backgroundExecutor is set after setExecutorProvider

Did you mean backgroundExecutor or backgroundExecutorProvider in your response?

Looks like setting backgroundExecutorProvider after setExecutorProvider must be an error (note, there is not need to preserve backward compabitility here, because backgroundExecutorProvider is just getting introduced, so setting is after executor is a non-existing scenario now, and if somebody actually tries to do that, they must know that they should clean up the legacy part first.

vam-google · 2021-06-03T23:52:02Z

gax/src/main/java/com/google/api/gax/rpc/ClientSettings.java

  public final ExecutorProvider getExecutorProvider() {
-    return stubSettings.getExecutorProvider();
+    return stubSettings.getBackgroundExecutorProvider();


Please add Java docs to all the public stuff regarding the newly introduced backgroundExecutor and explain clearly what types of executors may participate in a lifecycle of a call, and what exactly the backgroundExecutor is for. This is important, since we are adding a new entity/concept on the surface of gax.

Looks like other java docs for setter/getters are in the Builder. I updated the comments in the Builder (line 269 and line 282). Should we also have the doc here?

vam-google · 2021-06-03T23:58:36Z

gax/src/main/java/com/google/api/gax/rpc/StubSettings.java

@@ -101,10 +104,16 @@ protected StubSettings(Builder builder) {
    this.tracerFactory = builder.tracerFactory;
  }

+  /** @deprecated Please use {@link #getBackgroundExecutorProvider()}. */
+  @Deprecated


The wrapping logic in ClientSettings already calls StubSettings.getBackgroundExecutorProvider() for the getExecutorProvider() getter method (i.e. keeping the executorProvider as a thing on the surface for backward compatibility, but internally totally relying on backgroundExecutorProvider). Here the executorProvideris still maintained separately from backgroundExecutor provider. Should this class do the same (i.e. remove executorProvider field completely and for all the getter/setter methods just use backgroundExecutorProvider field instead?

backgroundExecutorProvider is default to InstantiatingExecutorProvider (in StubSettings#Builder: https://github.com/googleapis/gax-java/pull/1355/files#diff-1471e90108f24262d8e1b215bdd0fdff64d159d2201ba72bda5df6dba58fdc7cR262), and executorProvider is now default to null.

In the default case, when both StubSettings#setExecutor and TrasnportChannelProvider#withExecutor are not set, we want to use default grpc executor for transportChannelProvider, so we need to keep executorProvider to be null for the default case (the check in ClientContext#create https://github.com/googleapis/gax-java/pull/1355/files#diff-6138cc9cd4c9af4ec5aceebe7a723585298a068fff7a7848946c452778cca794R186). And since ClientContext#create checks with StubSettings#getExecutor, StubSettings#getExecutor still needs to return executorProvider.

Maybe ClientSettings#getExecutor should return executorProvider as well?

vam-google · 2021-06-04T00:00:42Z

gax/src/main/java/com/google/api/gax/rpc/StubSettings.java

    public B setExecutorProvider(ExecutorProvider executorProvider) {
+      // For backward compatibility, this will set both executorProvider and


Since executorProvider field is private and nobody knows about it except this class, can we instead of trying to keep the executorProvider and backgroundExecutorProvider in sync, just delete executorProvider and replace it with backgroundExecutorProvider everywhere (i.e. basically making executorProvider and backgroundExecutorProvider synonyms of the same thing, istead of being copies of the same thing)?

(Explained in the comment above):

backgroundExecutorProvider is default to InstantiatingExecutorProvider (in StubSettings#Builder: https://github.com/googleapis/gax-java/pull/1355/files#diff-1471e90108f24262d8e1b215bdd0fdff64d159d2201ba72bda5df6dba58fdc7cR262), and executorProvider is now default to null.

In the default case, when both StubSettings#setExecutor and TrasnportChannelProvider#withExecutor are not set, we want to use default grpc executor for transportChannelProvider, so we need to keep executorProvider to be null for the default case (the check in ClientContext#create https://github.com/googleapis/gax-java/pull/1355/files#diff-6138cc9cd4c9af4ec5aceebe7a723585298a068fff7a7848946c452778cca794R186).

vam-google · 2021-06-04T00:03:26Z

gax/src/main/java/com/google/api/gax/rpc/TransportChannelProvider.java

@@ -64,6 +64,7 @@
  boolean shouldAutoClose();

  /** True if the TransportProvider needs an executor. */
+  @Deprecated


Since it is the top interface of this needsExecutor thing, please add java doc explaining the reason of deprecation and what to do if you depend on it already.

mutianf · 2021-06-08T16:48:17Z

It is still not clear for me why the ultimate goal is to disable setting executor in the settings (i.e. stubSettings#setExecutorProvider()) and transportChannelProvider#withExecutor() must become the only way. Why can't we still keep both ways? I.e. it seems like relying on grpc default executor does not necessarily require deprecating the current gax's way of setting custom executor provider, if somebody wants to do so.

It seems like setExecutor() was meant to be used for scheduled api call logics. But because some transport channel (like httpjson) doesn't have a default executor, it's also shared with transport channel provider. And the default behavior became transport channel will share the same executors with other async call logic with a limited pool size. Instead, I think it makes more sense to make sure transport channel providers have their own default executors. So setExecutor() should be restricted to only set executors for async calls. For backward compatibility, I created setBackgroundExecutor for this purpose and deprecated setExecutor.

We could keep setExecutor just for setting executor for transport channel provider. But I think it might be a bit confusing to have 2 places to set the executor. And ClientContext will have setters and getters for both executor and backgroundExecutor, but executor is part of transport channel provider and not a top level context of the client, so it might be a bit confusing there too. The previous pr #869 didn't touch setExecutor() and I think was a simpler fix, but will have some compatibility issue as I mentioned in the pr's description.

vam-google

Looks good (especially after our discussion over GVC). Just a few minor comments and two more things please:

Please rename "executor" (too generic, now, used to make sense, where there was only one executor), where possible (i.e. non-breaking, private stuff) for it to reflect what it means now
Please double check the httpjson part - do we really need the default executor there? Note, none of the grpc-specific stuff is applicable to httpjson, and this PR exists because we want to use grpc executor service directly (which is not a thing for httpjson, so I'm surprised that that code is touched at all).

vam-google · 2021-07-26T06:42:12Z

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

+      Executors.newCachedThreadPool(
+          new ThreadFactoryBuilder()
+              .setDaemon(true)
+              .setNameFormat("http-default-executor-%d")


It is a name format for the threads created by the executor, not for the executor itself. Please remove "executor" from the name, and probably prefix it h as ttpjson, not http to match the naming pattern in gax-httpjson.

I think this is supposed to mirror grpc's naming scheme:
https://github.com/grpc/grpc-java/blob/438f8d9e7880b2f6ae2b376a35a9f5f32b4dbeaa/core/src/main/java/io/grpc/internal/GrpcUtil.java#L529

I updated this to use InstantiatingExecutorProvider, which will be the default for httpjson channels if no executor is set.

vam-google · 2021-07-26T06:43:35Z

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

      Preconditions.checkNotNull(endpoint);
+      if (executor == null) {


please set executor to default value on builder creation, but do not override it here (that is the idiomatic way of having default values in builder)

vam-google · 2021-07-26T06:46:10Z

gax/src/main/java/com/google/api/gax/rpc/ClientContext.java

    ExecutorProvider executorProvider = settings.getExecutorProvider();
-    final ScheduledExecutorService executor = executorProvider.getExecutor();
+    final ScheduledExecutorService executor =


to call this executor used to make sense, because there was only one executor. Now there are many different ones, so please name this variable what it actually is (not just a generic executor

vam-google · 2021-07-26T06:54:44Z

gax/src/main/java/com/google/api/gax/rpc/ClientContext.java

+    if (executorProvider != null
+        && executor != null
+        && executorProvider.shouldAutoClose()
+        && executor != backgroundExecutor) {


Did you mean backgroundExecutor or backgroundExecutorProvider in your response?

Looks like setting backgroundExecutorProvider after setExecutorProvider must be an error (note, there is not need to preserve backward compabitility here, because backgroundExecutorProvider is just getting introduced, so setting is after executor is a non-existing scenario now, and if somebody actually tries to do that, they must know that they should clean up the legacy part first.

vam-google · 2021-07-26T06:58:14Z

gax/src/main/java/com/google/api/gax/rpc/StubSettings.java

@@ -190,6 +200,7 @@ public String toString() {
      SettingsT extends StubSettings<SettingsT>, B extends Builder<SettingsT, B>> {

    private ExecutorProvider executorProvider;
+    private ExecutorProvider backgroundExecutorProvider;


It seems like what actually matters is to track the fact that somebody explicitly set executorProvider, but it should be impossible to have executorProvider != backgroundExecutorProvider (see the other comment about the scenario when sombebody sets backgroundExecutorProvider after executorProvider). Please, if possible, cleanup existence of both providers, and keep only one (and potentially keep track of executorProvider being set in the legacy way to preserve backward compatibility). This is to reduce the amount of executors and executor providers participating in gax, because they are a a complicated thing and keeping multiple ones very quickly complicates understanding of the code.

Updated the code to use a boolean to track if setExecutorProvider is called, and return null or backgroundExecutorProvider depending on the flag.

vam-google

LGTM this PR to avoid extra waiting time, but please address the latest comments first

deprecated executor

🤖 I have created a release \*beep\* \*boop\* --- ## [2.0.0](https://www.github.com/googleapis/gax-java/compare/v1.67.0...v2.0.0) (2021-07-30) ### Features * promote to 2.0.0 ([#1444](https://www.github.com/googleapis/gax-java/issues/1444)) ([776b1aa](https://www.github.com/googleapis/gax-java/commit/776b1aa73022bedec55e69732245b73cd04608f8)) ### Bug Fixes * stop overriding default grpc executor ([#1355](https://www.github.com/googleapis/gax-java/issues/1355)) ([b1f8c43](https://www.github.com/googleapis/gax-java/commit/b1f8c43cc90eb8e5ef78d142878841689356738c)) ### Dependencies * update api-common, guava, google-auth-library-credentials ([#1442](https://www.github.com/googleapis/gax-java/issues/1442)) ([2925ed7](https://www.github.com/googleapis/gax-java/commit/2925ed78cfb74db07a87da28839aeebc9027ac72)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 28, 2021

mutianf force-pushed the grpc_channel branch from d076e43 to 73bc3ce Compare April 28, 2021 21:25

igorbernstein2 suggested changes Apr 29, 2021

View reviewed changes

mutianf force-pushed the grpc_channel branch from 0e4d1c9 to dca05d3 Compare April 29, 2021 18:55

mutianf marked this pull request as ready for review April 29, 2021 21:16

mutianf requested review from a team as code owners April 29, 2021 21:16

igorbernstein2 approved these changes May 4, 2021

View reviewed changes

igorbernstein2 mentioned this pull request May 4, 2021

fix: stop overriding the default gRPC executor #869

Closed

mutianf force-pushed the grpc_channel branch from 8126d35 to fca5464 Compare June 3, 2021 17:30

vam-google reviewed Jun 4, 2021

View reviewed changes

vam-google reviewed Jul 26, 2021

View reviewed changes

vam-google approved these changes Jul 27, 2021

View reviewed changes

mutianf added 8 commits July 27, 2021 16:04

fix: do not override grpc default executor

2bf4a18

updates

feb3e4f

rebase and update some comments

97a7fb0

updates

226536f

fix formatting

f8b212d

update comments

8652952

update

d323f11

Keep default http json executor the same, use boolean to track

ea12683

deprecated executor

mutianf force-pushed the grpc_channel branch from cc041e7 to ea12683 Compare July 27, 2021 16:06

mutianf added 2 commits July 27, 2021 18:26

remove commented code

7efa9a7

Merge branch 'master' into grpc_channel

7a017cc

vam-google merged commit b1f8c43 into googleapis:master Jul 29, 2021

mutianf deleted the grpc_channel branch September 24, 2021 13:40

lqiu96 mentioned this pull request May 15, 2023

feat: Implement awaitTermination() for MangedHttpJsonChannel googleapis/sdk-platform-java#1677

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: stop overriding default grpc executor #1355

fix: stop overriding default grpc executor #1355

mutianf commented Apr 28, 2021 •

edited

codecov bot commented Apr 28, 2021 •

edited

igorbernstein2 left a comment

igorbernstein2 commented May 5, 2021

vam-google left a comment

vam-google Jun 3, 2021

vam-google Jun 3, 2021

vam-google Jun 3, 2021

mutianf Jun 4, 2021

vam-google Jun 3, 2021

vam-google Jun 3, 2021

mutianf Jun 7, 2021

vam-google Jul 26, 2021

vam-google Jun 3, 2021

mutianf Jun 7, 2021

vam-google Jun 3, 2021

mutianf Jun 7, 2021 •

edited

vam-google Jun 4, 2021

mutianf Jun 7, 2021 •

edited

vam-google Jun 4, 2021

mutianf commented Jun 8, 2021

vam-google left a comment

vam-google Jul 26, 2021

igorbernstein2 Jul 26, 2021

mutianf Jul 27, 2021

vam-google Jul 26, 2021

vam-google Jul 26, 2021

vam-google Jul 26, 2021

vam-google Jul 26, 2021

mutianf Jul 27, 2021

vam-google left a comment

		public B setExecutorProvider(ExecutorProvider executorProvider) {
		// For backward compatibility, this will set both executorProvider and

		Preconditions.checkNotNull(endpoint);
		if (executor == null) {

fix: stop overriding default grpc executor #1355

fix: stop overriding default grpc executor #1355

Conversation

mutianf commented Apr 28, 2021 • edited

codecov bot commented Apr 28, 2021 • edited

Codecov Report

igorbernstein2 left a comment

Choose a reason for hiding this comment

igorbernstein2 commented May 5, 2021

vam-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mutianf Jun 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mutianf Jun 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mutianf commented Jun 8, 2021

vam-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

mutianf commented Apr 28, 2021 •

edited

codecov bot commented Apr 28, 2021 •

edited

mutianf Jun 7, 2021 •

edited

mutianf Jun 7, 2021 •

edited