fix: stop overriding the default gRPC executor

[igorbernstein2]

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

[codecov]

Codecov Report

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

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

[vam-google]

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

[vam-google]

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

[vam-google]

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.

[igorbernstein2]

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

[vam-google]

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?

[igorbernstein2]

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]

Is this still open?

[igorbernstein2]

Closing in favor of #1355