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

fix: stop overriding the default gRPC executor #869

Closed

Conversation

igorbernstein2
Copy link
Contributor

@igorbernstein2 igorbernstein2 commented Feb 11, 2020

By default, gRPC uses a CachedThreadExecutor, which has unlimited
threads. However before this change, gax-java would override it with a
fixed 4 threaded ScheduledExecutor. This causes performance issues on
multicore machines.

This PR decouples the default gax executor from the default gRPC
executor, but retains the backwards compatibility so that the http json
channel provider still inherits the gax executor.

If a client developer or customer wants to override the gRPC executor,
they can still do so by calling
InstantiatingGrpcChannelProvide#withExecutor. However, since this
executor never needs to schedule tasks, a new method override was added
that accepts a simple Executor as opposed to ScheduledExecutorService.
This allows callers to use dynamically sized executors. Previous methods
have been deprecated.

There are still a couple of open questions:

  1. Should the http json transport provider be decoupled from the gax executor as well?
  2. In this PR TransportChannelProvider#withExecutor takes an Executor as opposed to an ExecutorService or an ExecutorProvider-like wrapper. This means that the TransportChannelProvider is not responsible for the lifecycle of the executor. This is similar to previous behavior when the customer changed the executor at the provider level. But more difficult than setting the gax executor.

Here is an example:

ExecutorService transportExecutor = Executors.newFixedThreadPool(32);

settings.stubSettings()
 .setTransportChannelProvider(
   settings.stubSettings().getTransportChanneProvider().toBuilder()
     .setExecutor(transportExecutor)
    .build()
 )

MyClient client = MyClient.create(settings.build());
// ... use client
client.close();

// have to explicitly shutdown the executor
transportExecutor.shutdown()

I'm not sure if I should add a TransportChannelExecutorProvider with Fixed & Instantiating impls to allow customers to dictate executor ownership. (note: the existing ExecutorProvider cannot be reused for this). But I'm leaning to towards not doing this to keep consist with grpc (which doesn't managed lifetimes of set executors) and to minimize the amount of code we have to maintain

By default, gRPC uses a CachedThreadExecutor, which has unlimited
threads. However before this change, gax-java would override it with a
fixed 4 threaded ScheduledExecutor. This causes performance issues on
multicore machines.

This PR decouples the default gax executor from the default gRPC
executor, but retains the backwards compatibility so that the http json
channel provider still inherits the gax executor.

If a client developer or customer wants to override the gRPC executor,
they cans till do so by calling
InstantiatingGrpcChannelProvide#withExecutor. However, since this
executor never needs to schedule tasks, a new method override was added
that accepts a simple Executor as opposed to ScheduledExecutorService.
This allows callers to use dynamically sized executors. Previous methods
have been deprecated.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 11, 2020
@codecov
Copy link

codecov bot commented Feb 11, 2020

Codecov Report

Merging #869 into master will increase coverage by 0.8%.
The diff coverage is 79.16%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #869     +/-   ##
===========================================
+ Coverage     78.62%   79.43%   +0.8%     
- Complexity     1163     1180     +17     
===========================================
  Files           203      203             
  Lines          5142     5150      +8     
  Branches        413      415      +2     
===========================================
+ Hits           4043     4091     +48     
+ Misses          925      880     -45     
- Partials        174      179      +5
Impacted Files Coverage Δ Complexity Δ
...gle/api/gax/rpc/FixedTransportChannelProvider.java 94.44% <0%> (-5.56%) 15 <0> (ø)
...ain/java/com/google/api/gax/rpc/ClientContext.java 80.88% <100%> (ø) 9 <0> (ø) ⬇️
...httpjson/InstantiatingHttpJsonChannelProvider.java 74.24% <75%> (+3.27%) 20 <4> (+1) ⬆️
...api/gax/grpc/InstantiatingGrpcChannelProvider.java 80.31% <90%> (+0.41%) 36 <3> (+1) ⬆️
...src/main/java/com/google/api/gax/rpc/Watchdog.java 80.34% <0%> (-0.17%) 11% <0%> (ø)
.../com/google/api/gax/grpc/GrpcTransportChannel.java 63.63% <0%> (+4.54%) 10% <0%> (+1%) ⬆️
...m/google/api/gax/httpjson/HttpRequestRunnable.java 60.41% <0%> (+6.25%) 6% <0%> (+1%) ⬆️
...e/api/gax/grpc/GrpcMetadataHandlerInterceptor.java 100% <0%> (+11.76%) 3% <0%> (+1%) ⬆️
...gle/api/gax/httpjson/HttpJsonTransportChannel.java 64.7% <0%> (+17.64%) 7% <0%> (+2%) ⬆️
... and 3 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 12a43ed...2fc552a. Read the comment docs.

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.

I can be easily missing something here (I haven't dived too deep into details), but I have a few concerns about breaking non-default behavior for users (i.e. cases, when people have configured their executors manually).

Also we need to make sure that we don't break manual clients, which depend on gax, like Spanner

final String expectedThreadName = "testExecutorOverrideExecutor";

ExecutorService executor =
Executors.newFixedThreadPool(
Copy link
Contributor

Choose a reason for hiding this comment

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

(Same comment as for the grpc tests). Please consider moving duplicated code in a helper method.

@@ -113,12 +113,25 @@ private InstantiatingGrpcChannelProvider(Builder builder) {

@Override
public boolean needsExecutor() {
return executorProvider == null;
// Use the default gRPC executor
return false;
Copy link
Contributor

@vam-google vam-google Feb 19, 2020

Choose a reason for hiding this comment

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

This looks strange. It seems like it can break all existing manual overrides of the executors people may have already introduced. I.e. this method is called in ClientContext.create() method to figure out if a "custom" executor from an executor should be used (passed as executorProvider in stub settings).

I guess in most cases it used to return true, which lead to using the executorProvider from stubSettings, which in its turn (unless user overrides it) leads to using the result of InstantiatingExecutorProvider.newBuilder().build();

I guess if we are talking about default behavior before this change and after this change it is ok (the default behavior will change, but the new default behavior is supposed to be better).

But if somebody really knew what they where doing and decided to override executorProvider in their stubSettings it seems like it will be ignored now, because transportChannelProvider.needsExecutor() in ClientContext.create() will always return false, ignoring the custom executor.

Copy link
Contributor Author

@igorbernstein2 igorbernstein2 Feb 20, 2020

Choose a reason for hiding this comment

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

This is the heart of the PR: the gax executor should not be shared with grpc. The PR proposes that channel executor should be configured on the channel providers instead of piping it through.

If you prefer to pipe it through, then the only approach I can think of is:
Add:

StubSettings#setTimerExecutor(ExecutorProvider)
StubSettings#setChannelExecutor(ChannelExecurtorProvider)

Have StubSettings#setExecutorProvider() call both the methods.

Add TransportChannelProvider#acceptsExecutor

Update ClientContext#create to have some logic like:

if (transportChannelProvider.needsExecutor() || (transportChannelProvider.acceptsExecutor() && channelExecutorProvider != null) {
   transportChannelProvider.withExecutor(channelExecutorProvider.get())
}

To me, this introduces a lot of complexity that is very hard to reason about

public Builder setExecutorProvider(ExecutorProvider executorProvider) {
Executor executor = null;
if (executorProvider != null) {
executor = executorProvider.getExecutor();
Copy link
Contributor

Choose a reason for hiding this comment

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

For InstantiatingExecutorProvider getExecutor() creates a new instance each time (for each channel, i guess). Calling it preemptively and only once basically converts InstantiatingExecutorProvider into a FixedExecutorProvider. This seems dangerous and may lead to extremely difficult to discover bugs.

Given that I'm not sure that I understand why the decision was made to "migrate" to explicitly setting Executor (i.e. adding setExecutor() and deprecating setExecutorProvider()). I.e. maybe we can keep the ExecutorProvider thing as is, and the only thing which will be changed is the default behavior to not overriding grpc's executor if user hasn't touched default settings for it in his client/stub settings?

Copy link
Contributor Author

@igorbernstein2 igorbernstein2 Feb 20, 2020

Choose a reason for hiding this comment

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

The common code path is:
ClientContext.create() -> transportProvider.withExecutor(executor) -> transportProvider.executorProvider = FixedExecutorProvider.create(executor)

So in most circumstances the behavior of channel sharing an executor is the same, which is good: you don't want a separate executor per channel.

The main behavior change is when the caller does something like:

stubSettings.setTransportProvider(
   InstantiatingGrpcChannelProvider.newBuilder()
     .setExecutorProvider(
         InstantiatingExecutorProvider.newBuilder().build()
     )
)

This would create an executor per channel, which is different behavior from

stubSettings.setExecutorProvider(
   InstantiatingExecutorProvider.newBuilder().build()
)

Which would share an executor across all channels

99% of the time you don't want a separate executor per channel

This change makes this explicit and removes ambiguity

@elharo
Copy link
Contributor

elharo commented Jan 12, 2021

Is this still open?

@igorbernstein2
Copy link
Contributor Author

Closing in favor of #1355

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.

4 participants