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.

Sign up for GitHub

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

Jump to bottom

feat: attemp DirectPath by default #467

Merged
merged 2 commits into from Oct 19, 2020
Merged

feat: attemp DirectPath by default #467

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
065fd56
feat: attemp DirectPath by default
WeiranFang Oct 14, 2020
d9bfde2
Add comment for DP attempt
WeiranFang Oct 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 4 additions & 23 deletions ...le/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java
View file
Open in desktop
Expand Up @@ -85,9 +85,6 @@ public class EnhancedBigtableStubSettings extends StubSettings<EnhancedBigtableS
private static final int MAX_MESSAGE_SIZE = 256 * 1024 * 1024;
private static final String SERVER_DEFAULT_APP_PROFILE_ID = "";

// TODO(weiranf): Remove this temporary endpoint once DirectPath goes to public beta
private static final String DIRECT_PATH_ENDPOINT = "test-bigtable.sandbox.googleapis.com:443";

private static final Set<Code> IDEMPOTENT_RETRY_CODES =
ImmutableSet.of(Code.DEADLINE_EXCEEDED, Code.UNAVAILABLE);

Expand Down Expand Up @@ -166,12 +163,6 @@ public class EnhancedBigtableStubSettings extends StubSettings<EnhancedBigtableS
private EnhancedBigtableStubSettings(Builder builder) {
super(builder);

if (DIRECT_PATH_ENDPOINT.equals(builder.getEndpoint())) {
logger.warning(
"Connecting to Bigtable using DirectPath."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there anywhere we can keep this logging information to confirm (in tests, for example) that we are connecting using DirectPath?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For tests we inject hooks into netty to ensure that directpath is used, also the integration tests generate verbose grpc logs which would show directpath usage. For prod usage, we dont really have a good way to introspect it as its a grpc level feature

+ " This is currently an experimental feature and should not be used in production.");
}

// Since point reads, streaming reads, bulk reads share the same base callable that converts
// grpc errors into ApiExceptions, they must have the same retry codes.
Preconditions.checkState(
Expand Down Expand Up @@ -245,13 +236,9 @@ public static InstantiatingGrpcChannelProvider.Builder defaultGrpcTransportProvi
.setKeepAliveTimeout(
Duration.ofSeconds(10)) // wait this long before considering the connection dead
.setKeepAliveWithoutCalls(true) // sends ping without active streams
// TODO(weiranf): Set this to true by default once DirectPath goes to public beta
.setAttemptDirectPath(isDirectPathEnabled());
}

// TODO(weiranf): Remove this once DirectPath goes to public beta
private static boolean isDirectPathEnabled() {
return Boolean.getBoolean("bigtable.attempt-directpath");
// Attempts direct access to CBT service over gRPC to improve throughput,
// whether the attempt is allowed is totally controlled by service owner.
.setAttemptDirectPath(true);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment on why we attemt DirectPath? to give context to future readers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment. PTAL at the wording, thanks!

}

static int getDefaultChannelPoolSize() {
Expand Down Expand Up @@ -526,13 +513,7 @@ private Builder() {
// Defaults provider
BigtableStubSettings.Builder baseDefaults = BigtableStubSettings.newBuilder();

// TODO(weiranf): remove this once DirectPath goes to public Beta and uses the default
// endpoint.
if (isDirectPathEnabled()) {
setEndpoint(DIRECT_PATH_ENDPOINT);
} else {
setEndpoint(baseDefaults.getEndpoint());
}
setEndpoint(baseDefaults.getEndpoint());

setTransportChannelProvider(defaultTransportChannelProvider());
setStreamWatchdogCheckInterval(baseDefaults.getStreamWatchdogCheckInterval());
Expand Down