-
Notifications
You must be signed in to change notification settings - Fork 69
fix: provide API to share the same background executor for channel po… #4030
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
60d2c1a
7c1892a
38aaa5a
98cad91
58dc3ee
b27e890
0538adf
77e5b15
eaec216
dbb5402
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,6 +129,7 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP | |
|
|
||
| private final int processorCount; | ||
| private final Executor executor; | ||
| @Nullable private final ScheduledExecutorService backgroundExecutor; | ||
| private final HeaderProvider headerProvider; | ||
| private final boolean useS2A; | ||
| private final String endpoint; | ||
|
|
@@ -181,6 +182,7 @@ public enum HardBoundTokenTypes { | |
| private InstantiatingGrpcChannelProvider(Builder builder) { | ||
| this.processorCount = builder.processorCount; | ||
| this.executor = builder.executor; | ||
| this.backgroundExecutor = builder.backgroundExecutor; | ||
| this.headerProvider = builder.headerProvider; | ||
| this.useS2A = builder.useS2A; | ||
| this.endpoint = builder.endpoint; | ||
|
|
@@ -245,6 +247,16 @@ public TransportChannelProvider withExecutor(Executor executor) { | |
| return toBuilder().setExecutor(executor).build(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean needsBackgroundExecutor() { | ||
| return backgroundExecutor == null; | ||
| } | ||
|
|
||
| @Override | ||
| public TransportChannelProvider withBackgroundExecutor(ScheduledExecutorService executor) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do we plan to use it? Do we expect customers to create an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ChannelProvider can have 2 executors: background executor and executor. Executor is used for handling rpc callbakcs. Background executor is used for all other background tasks. In the GrpcChannelProvider case, it doesn't need an executor because Grpc ManagedChannel provides a default executor that's optimized for performance. And the background executor is used for managing channels in the channel pool. Normally we don't want to use the same background executor for callbacks and background tasks because it impacts the performances. The settings you see in ClientContext is a bit confusing and it's from an old fix where we deprecated overriding the default grpc executor on the managed channel: 95c4c7b#diff-4e2f6e463b9b7d89de68e3f1a87765080045880c8dad018750f269e311f2471f. I don't think we actually go into the if statement by default because
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome, that's much easier to understand. Thanks! |
||
| return toBuilder().setBackgroundExecutor(executor).build(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean needsHeaders() { | ||
| return headerProvider == null; | ||
|
|
@@ -356,7 +368,9 @@ private TransportChannel createChannel() throws IOException { | |
| return GrpcTransportChannel.newBuilder() | ||
| .setManagedChannel( | ||
| ChannelPool.create( | ||
| channelPoolSettings, InstantiatingGrpcChannelProvider.this::createSingleChannel)) | ||
| channelPoolSettings, | ||
| InstantiatingGrpcChannelProvider.this::createSingleChannel, | ||
| backgroundExecutor)) | ||
| .setDirectPath(this.canUseDirectPath()) | ||
| .build(); | ||
| } | ||
|
|
@@ -839,6 +853,11 @@ public ChannelPoolSettings getChannelPoolSettings() { | |
| return channelPoolSettings; | ||
| } | ||
|
|
||
| /** Gets the background executor for channel refresh and resize. */ | ||
| ScheduledExecutorService getBackgroundExecutor() { | ||
| return backgroundExecutor; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean shouldAutoClose() { | ||
| return true; | ||
|
|
@@ -855,6 +874,7 @@ public static Builder newBuilder() { | |
| public static final class Builder { | ||
| @Deprecated private int processorCount; | ||
| private Executor executor; | ||
| private ScheduledExecutorService backgroundExecutor; | ||
| private HeaderProvider headerProvider; | ||
| private String endpoint; | ||
| private String mtlsEndpoint; | ||
|
|
@@ -891,6 +911,7 @@ private Builder() { | |
| private Builder(InstantiatingGrpcChannelProvider provider) { | ||
| this.processorCount = provider.processorCount; | ||
| this.executor = provider.executor; | ||
| this.backgroundExecutor = provider.backgroundExecutor; | ||
| this.headerProvider = provider.headerProvider; | ||
| this.endpoint = provider.endpoint; | ||
| this.useS2A = provider.useS2A; | ||
|
|
@@ -950,6 +971,15 @@ public Builder setExecutorProvider(ExecutorProvider executorProvider) { | |
| return setExecutor((Executor) executorProvider.getExecutor()); | ||
| } | ||
|
|
||
| /** | ||
| * Sets the background executor for this TransportChannelProvider. The life cycle of the | ||
| * executor should be managed by the caller. | ||
| */ | ||
| public Builder setBackgroundExecutor(ScheduledExecutorService executor) { | ||
| this.backgroundExecutor = executor; | ||
| return this; | ||
| } | ||
mutianf marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * Sets the HeaderProvider for this TransportChannelProvider. | ||
| * | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.