Skip to content
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

feat: enable channel priming by default #1555

Merged
merged 8 commits into from
Dec 16, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ public static Builder newBuilderForEmulator(String hostname, int port) {
.stubSettings()
.setCredentialsProvider(NoCredentialsProvider.create())
.setEndpoint(hostname + ":" + port)
// disable channel refreshing when creating an emulator
.setRefreshingChannel(false)
.setTransportChannelProvider(
InstantiatingGrpcChannelProvider.newBuilder()
.setMaxInboundMessageSize(256 * 1024 * 1024)
Expand Down Expand Up @@ -245,7 +247,6 @@ public String getAppProfileId() {
}

/** Gets if channels will gracefully refresh connections to Cloud Bigtable service */
@BetaApi("Channel priming is not currently stable and may change in the future")
public boolean isRefreshingChannel() {
return stubSettings.isRefreshingChannel();
}
Expand Down Expand Up @@ -395,19 +396,17 @@ public CredentialsProvider getCredentialsProvider() {
/**
* Configure periodic gRPC channel refreshes.
*
* <p>This feature will gracefully refresh connections to the Cloud Bigtable service. This is an
* experimental feature to address tail latency caused by the service dropping long lived gRPC
* connections, which causes the client to renegotiate the gRPC connection in the request path,
* which causes periodic spikes in latency
* <p>This feature will gracefully refresh connections to the Cloud Bigtable service. This is a
* feature to address tail latency caused by the service dropping long lived gRPC connections,
* which causes the client to renegotiate the gRPC connection in the request path, which causes
* periodic spikes in latency. This feature is enabled by default.
*/
@BetaApi("Channel priming is not currently stable and may change in the future")
public Builder setRefreshingChannel(boolean isRefreshingChannel) {
stubSettings.setRefreshingChannel(isRefreshingChannel);
return this;
}

/** Gets if channels will gracefully refresh connections to Cloud Bigtable service */
@BetaApi("Channel priming is not currently stable and may change in the future")
public boolean isRefreshingChannel() {
return stubSettings.isRefreshingChannel();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ static BigtableChannelPrimer create(
.setInstanceId(instanceId)
.setAppProfileId(appProfileId)
.setCredentialsProvider(FixedCredentialsProvider.create(credentials))
// Disable refreshing channel here to avoid creating settings in a loop
.setRefreshingChannel(false)
.setExecutorProvider(
InstantiatingExecutorProvider.newBuilder().setExecutorThreadCount(1).build());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/
package com.google.cloud.bigtable.data.v2.stub;

import com.google.api.core.BetaApi;
import com.google.api.core.InternalApi;
import com.google.api.gax.batching.BatchingCallSettings;
import com.google.api.gax.batching.BatchingSettings;
Expand Down Expand Up @@ -237,7 +236,6 @@ public String getAppProfileId() {
}

/** Returns if channels will gracefully refresh connections to Cloud Bigtable service */
@BetaApi("This API depends on experimental gRPC APIs")
public boolean isRefreshingChannel() {
return isRefreshingChannel;
}
Expand Down Expand Up @@ -545,7 +543,7 @@ public static class Builder extends StubSettings.Builder<EnhancedBigtableStubSet
*/
private Builder() {
this.appProfileId = SERVER_DEFAULT_APP_PROFILE_ID;
this.isRefreshingChannel = false;
this.isRefreshingChannel = true;
primedTableIds = ImmutableList.of();
jwtAudienceMapping = DEFAULT_JWT_AUDIENCE_MAPPING;
setCredentialsProvider(defaultCredentialsProviderBuilder().build());
Expand Down Expand Up @@ -757,11 +755,10 @@ public String getAppProfileId() {
* Sets if channels will gracefully refresh connections to Cloud Bigtable service.
*
* <p>When enabled, this will wait for the connection to complete the SSL handshake and warm up
* serverside caches for all the tables of the instance.
* serverside caches for all the tables of the instance. This feature is enabled by default.
*
* @see com.google.cloud.bigtable.data.v2.BigtableDataSettings.Builder#setRefreshingChannel
*/
mutianf marked this conversation as resolved.
Show resolved Hide resolved
@BetaApi("This API depends on experimental gRPC APIs")
public Builder setRefreshingChannel(boolean isRefreshingChannel) {
this.isRefreshingChannel = isRefreshingChannel;
return this;
Expand All @@ -778,7 +775,6 @@ public Builder setPrimedTableIds(String... tableIds) {
}

/** Gets if channels will gracefully refresh connections to Cloud Bigtable service */
@BetaApi("This API depends on experimental gRPC APIs")
public boolean isRefreshingChannel() {
return isRefreshingChannel;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ public void testToString() {
.setInstanceId("our-instance-85")
.setAppProfileId("our-appProfile-06")
.enableBatchMutationLatencyBasedThrottling(10)
// disable channel priming so we won't need authentication
// for sending the prime request since we're only testing the settings.
.setRefreshingChannel(false)
.build();
EnhancedBigtableStubSettings stubSettings = settings.getStubSettings();
assertThat(settings.toString())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ public void setUp() throws Exception {
.setProjectId(PROJECT_ID)
.setInstanceId(INSTANCE_ID)
.setCredentialsProvider(NoCredentialsProvider.create())
.setRefreshingChannel(false)
.build()
.getStubSettings();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,10 @@ public void readRowsIsNotLostTest() {
EnhancedBigtableStubSettings.Builder builder =
EnhancedBigtableStubSettings.newBuilder()
.setProjectId(dummyProjectId)
.setInstanceId(dummyInstanceId);
.setInstanceId(dummyInstanceId)
// Here and everywhere in this test, disable channel priming so we won't need
// authentication for sending the prime request since we're only testing the settings.
.setRefreshingChannel(false);

RetrySettings retrySettings =
RetrySettings.newBuilder()
Expand Down Expand Up @@ -243,7 +246,8 @@ public void readRowIsNotLostTest() {
EnhancedBigtableStubSettings.Builder builder =
EnhancedBigtableStubSettings.newBuilder()
.setProjectId("my-project")
.setInstanceId("my-instance");
.setInstanceId("my-instance")
.setRefreshingChannel(false);

RetrySettings retrySettings =
RetrySettings.newBuilder()
Expand Down Expand Up @@ -295,7 +299,8 @@ public void readRowRetryCodesMustMatch() {
EnhancedBigtableStubSettings.Builder builder =
EnhancedBigtableStubSettings.newBuilder()
.setProjectId("my-project")
.setInstanceId("my-instance");
.setInstanceId("my-instance")
.setRefreshingChannel(false);

builder.readRowsSettings().setRetryableCodes(Code.DEADLINE_EXCEEDED);

Expand Down Expand Up @@ -329,7 +334,8 @@ public void sampleRowKeysSettingsAreNotLostTest() {
EnhancedBigtableStubSettings.Builder builder =
EnhancedBigtableStubSettings.newBuilder()
.setProjectId(dummyProjectId)
.setInstanceId(dummyInstanceId);
.setInstanceId(dummyInstanceId)
.setRefreshingChannel(false);

RetrySettings retrySettings =
RetrySettings.newBuilder()
Expand Down Expand Up @@ -376,7 +382,8 @@ public void mutateRowSettingsAreNotLostTest() {
EnhancedBigtableStubSettings.Builder builder =
EnhancedBigtableStubSettings.newBuilder()
.setProjectId(dummyProjectId)
.setInstanceId(dummyInstanceId);
.setInstanceId(dummyInstanceId)
.setRefreshingChannel(false);

RetrySettings retrySettings =
RetrySettings.newBuilder()
Expand Down Expand Up @@ -423,7 +430,8 @@ public void bulkMutateRowsSettingsAreNotLostTest() {
EnhancedBigtableStubSettings.Builder builder =
EnhancedBigtableStubSettings.newBuilder()
.setProjectId(dummyProjectId)
.setInstanceId(dummyInstanceId);
.setInstanceId(dummyInstanceId)
.setRefreshingChannel(false);

assertThat(builder.bulkMutateRowsSettings().isLatencyBasedThrottlingEnabled()).isFalse();

Expand Down Expand Up @@ -536,7 +544,8 @@ public void bulkReadRowsSettingsAreNotLostTest() {
EnhancedBigtableStubSettings.Builder builder =
EnhancedBigtableStubSettings.newBuilder()
.setProjectId(dummyProjectId)
.setInstanceId(dummyInstanceId);
.setInstanceId(dummyInstanceId)
.setRefreshingChannel(false);

RetrySettings retrySettings =
RetrySettings.newBuilder()
Expand Down Expand Up @@ -611,7 +620,8 @@ public void checkAndMutateRowSettingsAreNotLostTest() {
EnhancedBigtableStubSettings.Builder builder =
EnhancedBigtableStubSettings.newBuilder()
.setProjectId(dummyProjectId)
.setInstanceId(dummyInstanceId);
.setInstanceId(dummyInstanceId)
.setRefreshingChannel(false);

RetrySettings retrySettings = RetrySettings.newBuilder().build();
builder
Expand Down Expand Up @@ -677,9 +687,7 @@ public void isRefreshingChannelDefaultValueTest() {
EnhancedBigtableStubSettings.newBuilder()
.setProjectId(dummyProjectId)
.setInstanceId(dummyInstanceId);
assertThat(builder.isRefreshingChannel()).isFalse();
assertThat(builder.build().isRefreshingChannel()).isFalse();
assertThat(builder.build().toBuilder().isRefreshingChannel()).isFalse();
assertThat(builder.isRefreshingChannel()).isTrue();
}

@Test
Expand Down Expand Up @@ -721,6 +729,7 @@ public void testToString() {
.setProjectId("our-project-85")
.setInstanceId("our-instance-06")
.setAppProfileId("our-appProfile-06")
.setRefreshingChannel(false)
.build();

checkToString(defaultSettings);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ public void testJwtAudience()
.setCredentialsProvider(FixedCredentialsProvider.create(jwtCreds))
.build();
enhancedBigtableStub = EnhancedBigtableStub.create(settings);

// Send rpc and grab the credentials sent
enhancedBigtableStub.readRowCallable().futureCall(Query.create("fake-table")).get();
Metadata metadata = metadataInterceptor.headers.take();
Expand Down Expand Up @@ -208,6 +207,9 @@ public void testBatchJwtAudience()
.setTransportChannelProvider(
FixedTransportChannelProvider.create(
GrpcTransportChannel.create(emulatorChannel)))
// Channel refreshing doesn't work with FixedTransportChannelProvider. Disable it for
// the test
.setRefreshingChannel(false)
.build();
enhancedBigtableStub = EnhancedBigtableStub.create(settings);
// Send rpc and grab the credentials sent
Expand Down Expand Up @@ -342,7 +344,7 @@ public void export(Collection<SpanData> collection) {
@Test
public void testBulkMutationFlowControllerConfigured() throws Exception {
BigtableDataSettings.Builder settings =
BigtableDataSettings.newBuilder()
BigtableDataSettings.newBuilderForEmulator(server.getPort())
.setProjectId("my-project")
.setInstanceId("my-instance")
.setCredentialsProvider(defaultSettings.getCredentialsProvider())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ public void checkAndMutateRow(
CheckAndMutateRowRequest request,
StreamObserver<CheckAndMutateRowResponse> responseObserver) {
responseObserver.onNext(CheckAndMutateRowResponse.getDefaultInstance());
responseObserver.onCompleted();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ public void setUp() throws IOException {
.setTransportChannelProvider(
FixedTransportChannelProvider.create(
GrpcTransportChannel.create(serverRule.getChannel())))
// channel priming doesn't work with FixedTransportChannelProvider. Disable it for the test
.setRefreshingChannel(false)
.build();

this.client = BigtableDataClient.create(settings.build());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ public void setUp() throws IOException {
.setTransportChannelProvider(
FixedTransportChannelProvider.create(
GrpcTransportChannel.create(serverRule.getChannel())))
// Refreshing channel doesn't work with FixedTransportChannelProvider
.setRefreshingChannel(false)
.build();

client = BigtableDataClient.create(settings.build());
Expand Down