diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableClientContext.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableClientContext.java index c4f882e2f7..12cab879fc 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableClientContext.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableClientContext.java @@ -33,6 +33,7 @@ import com.google.cloud.bigtable.data.v2.internal.csm.MetricsImpl; import com.google.cloud.bigtable.data.v2.internal.csm.attributes.ClientInfo; import com.google.cloud.bigtable.data.v2.internal.dp.AlwaysEnabledDirectAccessChecker; +import com.google.cloud.bigtable.data.v2.internal.dp.ClassicDirectAccessChecker; import com.google.cloud.bigtable.data.v2.internal.dp.DirectAccessChecker; import com.google.cloud.bigtable.data.v2.internal.dp.NoopDirectAccessChecker; import com.google.cloud.bigtable.data.v2.stub.metrics.CustomOpenTelemetryMetricsProvider; @@ -170,9 +171,14 @@ public static BigtableClientContext create( directAccessChecker = AlwaysEnabledDirectAccessChecker.INSTANCE; break; case FORCED_OFF: - case DEFAULT: directAccessChecker = NoopDirectAccessChecker.INSTANCE; break; + case DEFAULT: + default: + directAccessChecker = + new ClassicDirectAccessChecker( + metrics.getDirectPathCompatibleTracer(), channelPrimer, backgroundExecutor); + break; } BigtableTransportChannelProvider btTransportProvider = diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java index dcd0879b40..9a7944fbf3 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java @@ -182,7 +182,7 @@ public String getProjectId() { @InternalApi public DirectPathConfig getDirectPathConfig() { - return DIRECT_PATH_CONFIG; + return this.directPathConfig; } /** Returns the target instance id. */ @@ -637,7 +637,7 @@ private Builder() { // TODO: flip the bit setDirectAccessRequested and setTrafficDirectorEnabled once we make // client compatible by default. - boolean isDirectPathRequested = directPathConfig == DirectPathConfig.FORCED_ON; + boolean isDirectPathRequested = directPathConfig != DirectPathConfig.FORCED_OFF; featureFlags = FeatureFlags.newBuilder() .setReverseScans(true) diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/BigtableDataClientFactoryTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/BigtableDataClientFactoryTest.java index aa8ec31137..30a4853a61 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/BigtableDataClientFactoryTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/BigtableDataClientFactoryTest.java @@ -264,12 +264,103 @@ public void testCreateWithRefreshingChannel() throws Exception { .setInstanceId(DEFAULT_INSTANCE_ID) .setAppProfileId(DEFAULT_APP_PROFILE_ID) .setRefreshingChannel(true); + builder + .stubSettings() + .setCredentialsProvider(credentialsProvider) + .setStreamWatchdogProvider(watchdogProvider) + .setBackgroundExecutorProvider(executorProvider); + InstantiatingGrpcChannelProvider channelProvider = + (InstantiatingGrpcChannelProvider) builder.stubSettings().getTransportChannelProvider(); + InstantiatingGrpcChannelProvider.Builder channelProviderBuilder = channelProvider.toBuilder(); + channelProviderBuilder.setChannelPoolSettings(ChannelPoolSettings.staticallySized(poolSize)); + builder.stubSettings().setTransportChannelProvider(channelProviderBuilder.build()); + + BigtableDataClientFactory factory = BigtableDataClientFactory.create(builder.build()); + factory.createDefault(); + factory.createForAppProfile("other-appprofile"); + factory.createForInstance("other-project", "other-instance"); + + // Make sure that only 1 instance is created by each provider + // getCredentials was called twice, in patchCredentials and when creating the fixed credentials + // in BigtableClientContext + Mockito.verify(credentialsProvider, Mockito.times(2)).getCredentials(); + Mockito.verify(executorProvider, Mockito.times(1)).getExecutor(); + Mockito.verify(watchdogProvider, Mockito.times(1)).getWatchdog(); + assertThat(warmedChannels).hasSize(poolSize + 1); + assertThat(warmedChannels.values()).doesNotContain(false); + + // Wait for all the connections to close asynchronously + factory.close(); + long sleepTimeMs = 1000; + Thread.sleep(sleepTimeMs); + // Verify that all the channels are closed + assertThat(terminateAttributes).hasSize(poolSize + 1); + } + + @Test + public void testCreateWithRefreshingChannelWithDirectAccessByDefault() throws Exception { + int poolSize = 3; + // TODO: remove the suppression when setRefreshingChannel can be removed + @SuppressWarnings("deprecation") + BigtableDataSettings.Builder builder = + BigtableDataSettings.newBuilderForEmulator(server.getPort()) + .setProjectId(DEFAULT_PROJECT_ID) + .setInstanceId(DEFAULT_INSTANCE_ID) + .setAppProfileId(DEFAULT_APP_PROFILE_ID) + .setRefreshingChannel(true); + builder + .stubSettings() + .setCredentialsProvider(credentialsProvider) + .setStreamWatchdogProvider(watchdogProvider) + .setBackgroundExecutorProvider(executorProvider) + .setDirectPathConfig(EnhancedBigtableStubSettings.DirectPathConfig.DEFAULT); + InstantiatingGrpcChannelProvider channelProvider = + (InstantiatingGrpcChannelProvider) builder.stubSettings().getTransportChannelProvider(); + InstantiatingGrpcChannelProvider.Builder channelProviderBuilder = channelProvider.toBuilder(); + channelProviderBuilder.setChannelPoolSettings(ChannelPoolSettings.staticallySized(poolSize)); + builder.stubSettings().setTransportChannelProvider(channelProviderBuilder.build()); + + BigtableDataClientFactory factory = BigtableDataClientFactory.create(builder.build()); + factory.createDefault(); + factory.createForAppProfile("other-appprofile"); + factory.createForInstance("other-project", "other-instance"); + + // Make sure that only 1 instance is created by each provider + // getCredentials was called twice, in patchCredentials and when creating the fixed credentials + // in BigtableClientContext + Mockito.verify(credentialsProvider, Mockito.times(2)).getCredentials(); + Mockito.verify(executorProvider, Mockito.times(1)).getExecutor(); + Mockito.verify(watchdogProvider, Mockito.times(1)).getWatchdog(); + assertThat(warmedChannels).hasSize(poolSize + 1); + assertThat(warmedChannels.values()).doesNotContain(false); + + // Wait for all the connections to close asynchronously + factory.close(); + long sleepTimeMs = 1000; + Thread.sleep(sleepTimeMs); + // Verify that all the channels are closed + // If we have DEFAULT, it will add one channel temporily + assertThat(terminateAttributes).hasSize(poolSize + 1); + } + + @Test + public void testCreateWithRefreshingChannelDisableDirectAccess() throws Exception { + int poolSize = 3; + // TODO: remove the suppression when setRefreshingChannel can be removed + @SuppressWarnings("deprecation") + BigtableDataSettings.Builder builder = + BigtableDataSettings.newBuilderForEmulator(server.getPort()) + .setProjectId(DEFAULT_PROJECT_ID) + .setInstanceId(DEFAULT_INSTANCE_ID) + .setAppProfileId(DEFAULT_APP_PROFILE_ID) + .setRefreshingChannel(true); + builder .stubSettings() .setCredentialsProvider(credentialsProvider) .setStreamWatchdogProvider(watchdogProvider) .setBackgroundExecutorProvider(executorProvider) - .setDirectPathConfig(EnhancedBigtableStubSettings.DirectPathConfig.FORCED_ON); + .setDirectPathConfig(EnhancedBigtableStubSettings.DirectPathConfig.FORCED_OFF); InstantiatingGrpcChannelProvider channelProvider = (InstantiatingGrpcChannelProvider) builder.stubSettings().getTransportChannelProvider(); InstantiatingGrpcChannelProvider.Builder channelProviderBuilder = channelProvider.toBuilder(); @@ -295,6 +386,7 @@ public void testCreateWithRefreshingChannel() throws Exception { long sleepTimeMs = 1000; Thread.sleep(sleepTimeMs); // Verify that all the channels are closed + // If we have DEFAULT, it will add one channel temporily assertThat(terminateAttributes).hasSize(poolSize); } diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubTest.java index bfc94dea50..462699bbd0 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubTest.java @@ -549,11 +549,14 @@ public void testChannelPrimerConfigured() throws IOException { // TODO: remove the suppression once setRefreshingChannel can be removed @SuppressWarnings("deprecation") EnhancedBigtableStubSettings settings = - defaultSettings.toBuilder().setRefreshingChannel(true).build(); + defaultSettings.toBuilder() + .setRefreshingChannel(true) + .setDirectPathConfig(EnhancedBigtableStubSettings.DirectPathConfig.DEFAULT) + .build(); try (EnhancedBigtableStub ignored = EnhancedBigtableStub.create(settings)) { // direct access checker ping - assertThat(fakeDataService.pingRequests).hasSize(1); + assertThat(fakeDataService.pingRequests).hasSize(2); } }