Skip to content

Commit 80158c8

Browse files
refactor: reuse user-configured executor for user-callback dispatch
ShimImpl now sources the userCallbackExecutor from the StubSettings when the caller has configured one — either via InstantiatingGrpcChannelProvider's setExecutor (PR googleapis#13557 exposed the getter on TransportChannelProvider) or the legacy StubSettings#setExecutorProvider. Ownership follows the source: transport-set is borrowed by convention; the legacy ExecutorProvider honors shouldAutoClose(). When neither is set the existing behavior is preserved — ShimImpl owns a dedicated bigtable-callback-shim cached pool. Client.userCallbackExecutor widens from Resource<ExecutorService> to Resource<Executor> to accept the borrowed handle; downstream consumers already require only Executor. The standalone Client.create(ClientSettings) path is unchanged: ClientSettings carries no executor wiring to source from.
1 parent 62b89be commit 80158c8

3 files changed

Lines changed: 144 additions & 15 deletions

File tree

java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/api/Client.java

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import java.util.List;
5050
import java.util.Set;
5151
import java.util.WeakHashMap;
52+
import java.util.concurrent.Executor;
5253
import java.util.concurrent.ExecutorService;
5354
import java.util.concurrent.Executors;
5455
import java.util.concurrent.ScheduledExecutorService;
@@ -87,11 +88,12 @@ public class Client implements AutoCloseable {
8788
private final FeatureFlags featureFlags;
8889
private final ClientInfo clientInfo;
8990
private final Resource<ScheduledExecutorService> backgroundExecutor;
90-
// Drains the per-op SerializingExecutor. Cached pool so a blocked user callback does not starve
91-
// heartbeats, retry delays, or other vRPCs (which all run on backgroundExecutor).
92-
// TODO: source from the gax TransportChannelProvider so transport and user-callback dispatch
93-
// share the same pool. Blocked on missing APIs to extract the configured executor from gax.
94-
private final Resource<ExecutorService> userCallbackExecutor;
91+
// Drains the per-op SerializingExecutor. Sourced from the gax TransportChannelProvider /
92+
// StubSettings on the compat path (see ShimImpl.selectUserCallbackExecutor) so a user-configured
93+
// transport executor is reused for callback dispatch; otherwise a dedicated cached pool keeps a
94+
// blocked user callback from starving heartbeats, retry delays, or other vRPCs (which all run on
95+
// backgroundExecutor).
96+
private final Resource<Executor> userCallbackExecutor;
9597
// Hashed-wheel timer for heartbeat / deadline / watchdog / retry scheduling. Built over
9698
// backgroundExecutor (the timer's tick thread dispatches bodies onto it). Single tick thread per
9799
// Client, shared across every SessionPoolImpl.
@@ -187,7 +189,8 @@ public static Client create(ClientSettings settings) throws IOException {
187189
Resource.createOwned(metrics, metrics::close),
188190
Resource.createOwned(configManager, configManager::close),
189191
Resource.createOwned(backgroundExecutor, backgroundExecutor::shutdown),
190-
Resource.createOwned(userCallbackExecutor, () -> shutdownAndAwait(userCallbackExecutor)));
192+
Resource.<Executor>createOwned(
193+
userCallbackExecutor, () -> shutdownAndAwait(userCallbackExecutor)));
191194
}
192195

193196
public Client(
@@ -197,7 +200,7 @@ public Client(
197200
Resource<Metrics> metrics,
198201
Resource<ClientConfigurationManager> configManager,
199202
Resource<ScheduledExecutorService> bgExecutor,
200-
Resource<ExecutorService> userCallbackExecutor)
203+
Resource<Executor> userCallbackExecutor)
201204
throws IOException {
202205
this.featureFlags = featureFlags;
203206
this.clientInfo = clientInfo;

java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/compat/ShimImpl.java

Lines changed: 35 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
*/
1616
package com.google.cloud.bigtable.data.v2.internal.compat;
1717

18+
import com.google.api.gax.core.ExecutorProvider;
1819
import com.google.api.gax.grpc.ChannelPoolSettings;
1920
import com.google.api.gax.grpc.InstantiatingGrpcChannelProvider;
2021
import com.google.api.gax.rpc.StubSettings;
@@ -45,6 +46,7 @@
4546
import com.google.cloud.bigtable.data.v2.models.RowAdapter;
4647
import com.google.cloud.bigtable.data.v2.models.RowMutation;
4748
import com.google.cloud.bigtable.data.v2.stub.MetadataExtractorInterceptor;
49+
import com.google.common.annotations.VisibleForTesting;
4850
import com.google.common.base.Strings;
4951
import com.google.common.collect.ImmutableMap;
5052
import com.google.common.util.concurrent.ThreadFactoryBuilder;
@@ -55,6 +57,7 @@
5557
import java.io.IOException;
5658
import java.time.Duration;
5759
import java.util.Optional;
60+
import java.util.concurrent.Executor;
5861
import java.util.concurrent.ExecutorService;
5962
import java.util.concurrent.Executors;
6063
import java.util.concurrent.ScheduledExecutorService;
@@ -163,12 +166,9 @@ public static Shim create(
163166
featureFlags = featureFlags.toBuilder().setSessionsRequired(true).build();
164167
}
165168

166-
ExecutorService userCallbackExecutor =
167-
Executors.newCachedThreadPool(
168-
new ThreadFactoryBuilder()
169-
.setNameFormat("bigtable-callback-shim-%d")
170-
.setDaemon(true)
171-
.build());
169+
Resource<Executor> userCallbackExecutor =
170+
selectUserCallbackExecutor(
171+
stubSettings.getTransportChannelProvider(), stubSettings.getExecutorProvider());
172172

173173
Client client =
174174
new Client(
@@ -178,8 +178,7 @@ public static Shim create(
178178
Resource.createShared(metrics),
179179
Resource.createShared(configManager),
180180
Resource.createShared(bgExecutor),
181-
Resource.createOwned(
182-
userCallbackExecutor, () -> Client.shutdownAndAwait(userCallbackExecutor)));
181+
userCallbackExecutor);
183182

184183
return new ShimImpl(configManager, client);
185184
}
@@ -192,6 +191,34 @@ public ShimImpl(ClientConfigurationManager configManager, Client client) {
192191
this.mutateRowShim = new MutateRowShim(client);
193192
}
194193

194+
// If the user configured an executor — either via InstantiatingGrpcChannelProvider#setExecutor
195+
// or the legacy StubSettings#setExecutorProvider — reuse it for user-callback dispatch so the
196+
// transport pool and callback pool are the same. Ownership: transport-set is borrowed by
197+
// convention; legacy provider honors shouldAutoClose(). Otherwise allocate a dedicated cached
198+
// pool that this Client owns.
199+
@VisibleForTesting
200+
static Resource<Executor> selectUserCallbackExecutor(
201+
TransportChannelProvider transportProvider, @Nullable ExecutorProvider executorProvider) {
202+
Executor transportExecutor = transportProvider.getExecutor();
203+
if (transportExecutor != null) {
204+
return Resource.createShared(transportExecutor);
205+
}
206+
if (executorProvider != null) {
207+
ScheduledExecutorService executor = executorProvider.getExecutor();
208+
if (executorProvider.shouldAutoClose()) {
209+
return Resource.createOwned(executor, () -> Client.shutdownAndAwait(executor));
210+
}
211+
return Resource.createShared(executor);
212+
}
213+
ExecutorService owned =
214+
Executors.newCachedThreadPool(
215+
new ThreadFactoryBuilder()
216+
.setNameFormat("bigtable-callback-shim-%d")
217+
.setDaemon(true)
218+
.build());
219+
return Resource.createOwned(owned, () -> Client.shutdownAndAwait(owned));
220+
}
221+
195222
private static ChannelProvider configureChannelProvider(
196223
ChannelProvider channelProvider, Metrics metrics) {
197224
return new ConfiguredChannelProvider(
Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
/*
2+
* Copyright 2026 Google LLC
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package com.google.cloud.bigtable.data.v2.internal.compat;
17+
18+
import static com.google.common.truth.Truth.assertThat;
19+
import static org.mockito.Mockito.mock;
20+
import static org.mockito.Mockito.never;
21+
import static org.mockito.Mockito.verify;
22+
23+
import com.google.api.gax.core.FixedExecutorProvider;
24+
import com.google.api.gax.grpc.InstantiatingGrpcChannelProvider;
25+
import com.google.cloud.bigtable.data.v2.internal.api.Client.Resource;
26+
import java.util.concurrent.CountDownLatch;
27+
import java.util.concurrent.Executor;
28+
import java.util.concurrent.ExecutorService;
29+
import java.util.concurrent.ScheduledExecutorService;
30+
import java.util.concurrent.TimeUnit;
31+
import org.junit.Test;
32+
33+
public class ShimImplTest {
34+
35+
private static final InstantiatingGrpcChannelProvider EMPTY_TRANSPORT =
36+
InstantiatingGrpcChannelProvider.newBuilder().build();
37+
38+
@Test
39+
public void selectUserCallbackExecutor_transportSet_returnsSharedTransportExecutor() {
40+
Executor userExec = mock(Executor.class);
41+
InstantiatingGrpcChannelProvider transport =
42+
InstantiatingGrpcChannelProvider.newBuilder().setExecutor(userExec).build();
43+
44+
Resource<Executor> resource = ShimImpl.selectUserCallbackExecutor(transport, null);
45+
46+
assertThat(resource.get()).isSameInstanceAs(userExec);
47+
// Shared resources have a no-op closer; close() must not throw.
48+
resource.close();
49+
}
50+
51+
@Test
52+
public void selectUserCallbackExecutor_legacyNoAutoClose_returnsSharedExecutor() {
53+
ScheduledExecutorService userExec = mock(ScheduledExecutorService.class);
54+
55+
Resource<Executor> resource =
56+
ShimImpl.selectUserCallbackExecutor(
57+
EMPTY_TRANSPORT,
58+
FixedExecutorProvider.create(userExec, /* shouldAutoClose= */ false));
59+
60+
assertThat(resource.get()).isSameInstanceAs(userExec);
61+
resource.close();
62+
verify(userExec, never()).shutdown();
63+
}
64+
65+
@Test
66+
public void selectUserCallbackExecutor_legacyAutoClose_returnsOwnedExecutor() {
67+
ScheduledExecutorService userExec = mock(ScheduledExecutorService.class);
68+
69+
Resource<Executor> resource =
70+
ShimImpl.selectUserCallbackExecutor(
71+
EMPTY_TRANSPORT,
72+
FixedExecutorProvider.create(userExec, /* shouldAutoClose= */ true));
73+
74+
assertThat(resource.get()).isSameInstanceAs(userExec);
75+
resource.close();
76+
verify(userExec).shutdown();
77+
}
78+
79+
@Test
80+
public void selectUserCallbackExecutor_unset_allocatesOwnedCachedPool() throws Exception {
81+
Resource<Executor> resource = ShimImpl.selectUserCallbackExecutor(EMPTY_TRANSPORT, null);
82+
83+
Executor exec = resource.get();
84+
assertThat(exec).isInstanceOf(ExecutorService.class);
85+
86+
String[] threadName = new String[1];
87+
CountDownLatch done = new CountDownLatch(1);
88+
exec.execute(
89+
() -> {
90+
threadName[0] = Thread.currentThread().getName();
91+
done.countDown();
92+
});
93+
assertThat(done.await(5, TimeUnit.SECONDS)).isTrue();
94+
assertThat(threadName[0]).startsWith("bigtable-callback-shim-");
95+
96+
resource.close();
97+
assertThat(((ExecutorService) exec).isShutdown()).isTrue();
98+
}
99+
}

0 commit comments

Comments
 (0)