Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

fix: stop overriding default grpc executor #1355

Merged
merged 10 commits into from
Jul 29, 2021

Conversation

mutianf
Copy link
Contributor

@mutianf mutianf commented Apr 28, 2021

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:

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

  2. 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:

  1. Add a backgroundExecutorProvider in StubSettings, and use a boolean to track if ClientSettings#setExecutorProvider() is called.
  2. 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().

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

codecov bot commented Apr 28, 2021

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
Impacted file tree graph

@@             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.

@mutianf mutianf marked this pull request as ready for review April 29, 2021 21:16
@mutianf mutianf requested review from a team as code owners April 29, 2021 21:16
Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@igorbernstein2
Copy link
Contributor

@vam-google ping

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Few more things, not mentioned in the individual comments:

  1. 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).

  2. 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.

try {
Channel channel = transportChannel.getChannel();

ClientCall<com.google.type.Color, Money> call =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.build();

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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 =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

public Builder setExecutor(Executor executor) {
this.executor = executor;
if (executor != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

if (executorProvider != null
&& executor != null
&& executorProvider.shouldAutoClose()
&& executor != backgroundExecutor) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public final ExecutorProvider getExecutorProvider() {
return stubSettings.getExecutorProvider();
return stubSettings.getBackgroundExecutorProvider();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@mutianf mutianf Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

@mutianf mutianf Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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).

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

/** True if the TransportProvider needs an executor. */
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

mutianf commented Jun 8, 2021

  1. 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.

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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
  2. 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).

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

ExecutorProvider executorProvider = settings.getExecutorProvider();
final ScheduledExecutorService executor = executorProvider.getExecutor();
final ScheduledExecutorService executor =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (executorProvider != null
&& executor != null
&& executorProvider.shouldAutoClose()
&& executor != backgroundExecutor) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

private ExecutorProvider executorProvider;
private ExecutorProvider backgroundExecutorProvider;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants