Skip to content

Commit bffd892

Browse files
sushanbsushanb
authored
feat(bigtable): enable direct access by default (#2857)
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [ ] Make sure to open an issue as a [bug/issue](https://togithub.com/googleapis/java-bigtable/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) - [ ] Rollback plan is reviewed and LGTMed - [ ] All new data plane features have a completed end to end testing plan Fixes #<issue_number_goes_here> ☕️ If you write sample code, please follow the [samples format]( https://togithub.com/GoogleCloudPlatform/java-docs-samples/blob/main/SAMPLE_FORMAT.md).
1 parent 9345b35 commit bffd892

4 files changed

Lines changed: 107 additions & 6 deletions

File tree

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableClientContext.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import com.google.cloud.bigtable.data.v2.internal.csm.MetricsImpl;
3434
import com.google.cloud.bigtable.data.v2.internal.csm.attributes.ClientInfo;
3535
import com.google.cloud.bigtable.data.v2.internal.dp.AlwaysEnabledDirectAccessChecker;
36+
import com.google.cloud.bigtable.data.v2.internal.dp.ClassicDirectAccessChecker;
3637
import com.google.cloud.bigtable.data.v2.internal.dp.DirectAccessChecker;
3738
import com.google.cloud.bigtable.data.v2.internal.dp.NoopDirectAccessChecker;
3839
import com.google.cloud.bigtable.data.v2.stub.metrics.CustomOpenTelemetryMetricsProvider;
@@ -170,9 +171,14 @@ public static BigtableClientContext create(
170171
directAccessChecker = AlwaysEnabledDirectAccessChecker.INSTANCE;
171172
break;
172173
case FORCED_OFF:
173-
case DEFAULT:
174174
directAccessChecker = NoopDirectAccessChecker.INSTANCE;
175175
break;
176+
case DEFAULT:
177+
default:
178+
directAccessChecker =
179+
new ClassicDirectAccessChecker(
180+
metrics.getDirectPathCompatibleTracer(), channelPrimer, backgroundExecutor);
181+
break;
176182
}
177183

178184
BigtableTransportChannelProvider btTransportProvider =

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ public String getProjectId() {
182182

183183
@InternalApi
184184
public DirectPathConfig getDirectPathConfig() {
185-
return DIRECT_PATH_CONFIG;
185+
return this.directPathConfig;
186186
}
187187

188188
/** Returns the target instance id. */
@@ -637,7 +637,7 @@ private Builder() {
637637

638638
// TODO: flip the bit setDirectAccessRequested and setTrafficDirectorEnabled once we make
639639
// client compatible by default.
640-
boolean isDirectPathRequested = directPathConfig == DirectPathConfig.FORCED_ON;
640+
boolean isDirectPathRequested = directPathConfig != DirectPathConfig.FORCED_OFF;
641641
featureFlags =
642642
FeatureFlags.newBuilder()
643643
.setReverseScans(true)

‎google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/BigtableDataClientFactoryTest.java‎

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,12 +264,103 @@ public void testCreateWithRefreshingChannel() throws Exception {
264264
.setInstanceId(DEFAULT_INSTANCE_ID)
265265
.setAppProfileId(DEFAULT_APP_PROFILE_ID)
266266
.setRefreshingChannel(true);
267+
builder
268+
.stubSettings()
269+
.setCredentialsProvider(credentialsProvider)
270+
.setStreamWatchdogProvider(watchdogProvider)
271+
.setBackgroundExecutorProvider(executorProvider);
272+
InstantiatingGrpcChannelProvider channelProvider =
273+
(InstantiatingGrpcChannelProvider) builder.stubSettings().getTransportChannelProvider();
274+
InstantiatingGrpcChannelProvider.Builder channelProviderBuilder = channelProvider.toBuilder();
275+
channelProviderBuilder.setChannelPoolSettings(ChannelPoolSettings.staticallySized(poolSize));
276+
builder.stubSettings().setTransportChannelProvider(channelProviderBuilder.build());
277+
278+
BigtableDataClientFactory factory = BigtableDataClientFactory.create(builder.build());
279+
factory.createDefault();
280+
factory.createForAppProfile("other-appprofile");
281+
factory.createForInstance("other-project", "other-instance");
282+
283+
// Make sure that only 1 instance is created by each provider
284+
// getCredentials was called twice, in patchCredentials and when creating the fixed credentials
285+
// in BigtableClientContext
286+
Mockito.verify(credentialsProvider, Mockito.times(2)).getCredentials();
287+
Mockito.verify(executorProvider, Mockito.times(1)).getExecutor();
288+
Mockito.verify(watchdogProvider, Mockito.times(1)).getWatchdog();
289+
assertThat(warmedChannels).hasSize(poolSize + 1);
290+
assertThat(warmedChannels.values()).doesNotContain(false);
291+
292+
// Wait for all the connections to close asynchronously
293+
factory.close();
294+
long sleepTimeMs = 1000;
295+
Thread.sleep(sleepTimeMs);
296+
// Verify that all the channels are closed
297+
assertThat(terminateAttributes).hasSize(poolSize + 1);
298+
}
299+
300+
@Test
301+
public void testCreateWithRefreshingChannelWithDirectAccessByDefault() throws Exception {
302+
int poolSize = 3;
303+
// TODO: remove the suppression when setRefreshingChannel can be removed
304+
@SuppressWarnings("deprecation")
305+
BigtableDataSettings.Builder builder =
306+
BigtableDataSettings.newBuilderForEmulator(server.getPort())
307+
.setProjectId(DEFAULT_PROJECT_ID)
308+
.setInstanceId(DEFAULT_INSTANCE_ID)
309+
.setAppProfileId(DEFAULT_APP_PROFILE_ID)
310+
.setRefreshingChannel(true);
311+
builder
312+
.stubSettings()
313+
.setCredentialsProvider(credentialsProvider)
314+
.setStreamWatchdogProvider(watchdogProvider)
315+
.setBackgroundExecutorProvider(executorProvider)
316+
.setDirectPathConfig(EnhancedBigtableStubSettings.DirectPathConfig.DEFAULT);
317+
InstantiatingGrpcChannelProvider channelProvider =
318+
(InstantiatingGrpcChannelProvider) builder.stubSettings().getTransportChannelProvider();
319+
InstantiatingGrpcChannelProvider.Builder channelProviderBuilder = channelProvider.toBuilder();
320+
channelProviderBuilder.setChannelPoolSettings(ChannelPoolSettings.staticallySized(poolSize));
321+
builder.stubSettings().setTransportChannelProvider(channelProviderBuilder.build());
322+
323+
BigtableDataClientFactory factory = BigtableDataClientFactory.create(builder.build());
324+
factory.createDefault();
325+
factory.createForAppProfile("other-appprofile");
326+
factory.createForInstance("other-project", "other-instance");
327+
328+
// Make sure that only 1 instance is created by each provider
329+
// getCredentials was called twice, in patchCredentials and when creating the fixed credentials
330+
// in BigtableClientContext
331+
Mockito.verify(credentialsProvider, Mockito.times(2)).getCredentials();
332+
Mockito.verify(executorProvider, Mockito.times(1)).getExecutor();
333+
Mockito.verify(watchdogProvider, Mockito.times(1)).getWatchdog();
334+
assertThat(warmedChannels).hasSize(poolSize + 1);
335+
assertThat(warmedChannels.values()).doesNotContain(false);
336+
337+
// Wait for all the connections to close asynchronously
338+
factory.close();
339+
long sleepTimeMs = 1000;
340+
Thread.sleep(sleepTimeMs);
341+
// Verify that all the channels are closed
342+
// If we have DEFAULT, it will add one channel temporily
343+
assertThat(terminateAttributes).hasSize(poolSize + 1);
344+
}
345+
346+
@Test
347+
public void testCreateWithRefreshingChannelDisableDirectAccess() throws Exception {
348+
int poolSize = 3;
349+
// TODO: remove the suppression when setRefreshingChannel can be removed
350+
@SuppressWarnings("deprecation")
351+
BigtableDataSettings.Builder builder =
352+
BigtableDataSettings.newBuilderForEmulator(server.getPort())
353+
.setProjectId(DEFAULT_PROJECT_ID)
354+
.setInstanceId(DEFAULT_INSTANCE_ID)
355+
.setAppProfileId(DEFAULT_APP_PROFILE_ID)
356+
.setRefreshingChannel(true);
357+
267358
builder
268359
.stubSettings()
269360
.setCredentialsProvider(credentialsProvider)
270361
.setStreamWatchdogProvider(watchdogProvider)
271362
.setBackgroundExecutorProvider(executorProvider)
272-
.setDirectPathConfig(EnhancedBigtableStubSettings.DirectPathConfig.FORCED_ON);
363+
.setDirectPathConfig(EnhancedBigtableStubSettings.DirectPathConfig.FORCED_OFF);
273364
InstantiatingGrpcChannelProvider channelProvider =
274365
(InstantiatingGrpcChannelProvider) builder.stubSettings().getTransportChannelProvider();
275366
InstantiatingGrpcChannelProvider.Builder channelProviderBuilder = channelProvider.toBuilder();
@@ -295,6 +386,7 @@ public void testCreateWithRefreshingChannel() throws Exception {
295386
long sleepTimeMs = 1000;
296387
Thread.sleep(sleepTimeMs);
297388
// Verify that all the channels are closed
389+
// If we have DEFAULT, it will add one channel temporily
298390
assertThat(terminateAttributes).hasSize(poolSize);
299391
}
300392

‎google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubTest.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -549,11 +549,14 @@ public void testChannelPrimerConfigured() throws IOException {
549549
// TODO: remove the suppression once setRefreshingChannel can be removed
550550
@SuppressWarnings("deprecation")
551551
EnhancedBigtableStubSettings settings =
552-
defaultSettings.toBuilder().setRefreshingChannel(true).build();
552+
defaultSettings.toBuilder()
553+
.setRefreshingChannel(true)
554+
.setDirectPathConfig(EnhancedBigtableStubSettings.DirectPathConfig.DEFAULT)
555+
.build();
553556

554557
try (EnhancedBigtableStub ignored = EnhancedBigtableStub.create(settings)) {
555558
// direct access checker ping
556-
assertThat(fakeDataService.pingRequests).hasSize(1);
559+
assertThat(fakeDataService.pingRequests).hasSize(2);
557560
}
558561
}
559562

0 commit comments

Comments
 (0)