Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

feat: add google-c2p dependence to DirectPath #1521

Merged
merged 11 commits into from Oct 24, 2021
Merged

feat: add google-c2p dependence to DirectPath #1521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
119b6d1
feat: add google-c2p dependence to DirectPath
mohanli-ml Oct 14, 2021
ce1227a
feat: add google-c2p dependence to DirectPath
mohanli-ml Oct 15, 2021
d8188d3
feat: add google-c2p dependence to DirectPath
mohanli-ml Oct 15, 2021
8bfcd10
feat: add google-c2p dependence to DirectPath
mohanli-ml Oct 19, 2021
77e81d7
feat: add google-c2p dependence to DirectPath
mohanli-ml Oct 19, 2021
70f3af2
Merge branch 'main' into add-google-c2p
mohanli-ml Oct 19, 2021
90ed61e
Merge branch 'main' into add-google-c2p
chanseokoh Oct 20, 2021
baaff18
Simplify code
chanseokoh Oct 20, 2021
db57e67
Simplify
chanseokoh Oct 20, 2021
1c288e0
feat: add google-c2p dependence to DirectPath
mohanli-ml Oct 20, 2021
c863608
feat: add google-c2p dependence to DirectPath
mohanli-ml Oct 21, 2021
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
1 change: 1 addition & 0 deletions build.gradle
View file
Open in desktop
Expand Up @@ -40,6 +40,7 @@ ext {
'maven.io_grpc_grpc_protobuf': "io.grpc:grpc-protobuf:${libraries['version.io_grpc']}",
'maven.io_grpc_grpc_netty_shaded': "io.grpc:grpc-netty-shaded:${libraries['version.io_grpc']}",
'maven.io_grpc_grpc_alts': "io.grpc:grpc-alts:${libraries['version.io_grpc']}",
'maven.io_grpc_grpc_xds': "io.grpc:grpc-xds:${libraries['version.io_grpc']}",
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that this should be added to dependencies.properties, not here, so that both Bazel and Gradle resolve and use the dependency.

However, the Bazel build didn't break, meaning that this is unnecessary. I also just compiled the project with Gradle manually without this, and it compiled successfully. In fact, I don't see any new class being used in the code below. Obviously, this dependency is not required for compilation. Should we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just noticed that this should be added to dependencies.properties, not here, so that both Bazel and Gradle resolve and use the dependency.

Can you give me an example about how to add the dependency to dependencies.properties? And should I also remove change in both gax-grpc/build.gradle?

However, the Bazel build didn't break, meaning that this is unnecessary. I also just compiled the project with Gradle manually without this, and it compiled successfully. In fact, I don't see any new class being used in the code below. Obviously, this dependency is not required for compilation. Should we remove it?

Compilation can succeed because as you said, no new class is added. However, this dependency is actually registering google-c2p resolver to the build binary, otherwise gRPC can not recognize the google-c2p:/// URI scheme and we will get error like Java.lang.IllegalArgumentException: cannot find a NameResolver for google-c2p:///localhost. So we should keep the dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. What you are saying sounds like the dependency is required to exist in the runtime classpath of the user consuming the gax-java library.

Since it's not required for compiling the project, let's have this as a Gradle-only dependency. I'm pretty new to this repo, so I don't know whether the Bazel build will also need this for something. However, gax-java libraries are released from Gradle, at least they will be okay. If there comes an issue on the Bazel side later, we'll take care of it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks!

'maven.com_google_protobuf': "com.google.protobuf:protobuf-java:${libraries['version.com_google_protobuf']}",
'maven.com_google_protobuf_java_util': "com.google.protobuf:protobuf-java-util:${libraries['version.com_google_protobuf']}")
}
Expand Down
2 changes: 2 additions & 0 deletions gax-grpc/build.gradle
View file
Open in desktop
Expand Up @@ -20,6 +20,8 @@ dependencies {
libraries['maven.io_grpc_grpc_protobuf'],
libraries['maven.io_grpc_grpc_stub'])

runtimeOnly libraries['maven.io_grpc_grpc_xds']

compileOnly libraries['maven.com_google_auto_value_auto_value']

testImplementation( project(':gax').sourceSets.test.output,
Expand Down
21 changes: 16 additions & 5 deletions gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Expand Up @@ -83,6 +83,7 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP
static final String DIRECT_PATH_ENV_VAR = "GOOGLE_CLOUD_ENABLE_DIRECT_PATH";
private static final String DIRECT_PATH_ENV_DISABLE_DIRECT_PATH =
"GOOGLE_CLOUD_DISABLE_DIRECT_PATH";
private static final String DIRECT_PATH_ENV_ENABLE_XDS = "GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS";
static final long DIRECT_PATH_KEEP_ALIVE_TIME_SECONDS = 3600;
static final long DIRECT_PATH_KEEP_ALIVE_TIMEOUT_SECONDS = 20;
// reduce the thundering herd problem of too many channels trying to (re)connect at the same time
Expand Down Expand Up @@ -330,16 +331,23 @@ private ManagedChannel createSingleChannel() throws IOException, GeneralSecurity

ManagedChannelBuilder<?> builder;

// TODO(weiranf): Add API in ComputeEngineCredentials to check default service account.
// Check DirectPath traffic.
boolean isDirectPathXdsEnabled = false;
if (isDirectPathEnabled(serviceAddress)
&& isNonDefaultServiceAccountAllowed()
&& isOnComputeEngine()) {
builder = ComputeEngineChannelBuilder.forAddress(serviceAddress, port);
isDirectPathXdsEnabled = Boolean.parseBoolean(envProvider.getenv(DIRECT_PATH_ENV_ENABLE_XDS));
if (isDirectPathXdsEnabled) {
// google-c2p resolver target must not have a port number
builder = ComputeEngineChannelBuilder.forTarget("google-c2p:///" + serviceAddress);
} else {
builder = ComputeEngineChannelBuilder.forAddress(serviceAddress, port);
builder.defaultServiceConfig(directPathServiceConfig);
}
// Set default keepAliveTime and keepAliveTimeout when directpath environment is enabled.
// Will be overridden by user defined values if any.
builder.keepAliveTime(DIRECT_PATH_KEEP_ALIVE_TIME_SECONDS, TimeUnit.SECONDS);
builder.keepAliveTimeout(DIRECT_PATH_KEEP_ALIVE_TIMEOUT_SECONDS, TimeUnit.SECONDS);
builder.defaultServiceConfig(directPathServiceConfig);
} else {
ChannelCredentials channelCredentials = createMtlsChannelCredentials();
if (channelCredentials != null) {
Expand All @@ -348,10 +356,13 @@ && isOnComputeEngine()) {
builder = ManagedChannelBuilder.forAddress(serviceAddress, port);
}
}
// google-c2p resolver requires service config lookup
if (!isDirectPathXdsEnabled) {
// See https://github.com/googleapis/gapic-generator/issues/2816
builder.disableServiceConfigLookUp();
}
builder =
builder
// See https://github.com/googleapis/gapic-generator/issues/2816
.disableServiceConfigLookUp()
.intercept(new GrpcChannelUUIDInterceptor())
.intercept(headerInterceptor)
.intercept(metadataHandlerInterceptor)
Expand Down
45 changes: 17 additions & 28 deletions gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
View file
Open in desktop
Expand Up @@ -67,6 +67,7 @@

@RunWith(JUnit4.class)
public class InstantiatingGrpcChannelProviderTest extends AbstractMtlsTransportChannelTest {

@Test
public void testEndpoint() {
String endpoint = "localhost:8080";
Expand Down Expand Up @@ -164,11 +165,8 @@ public void testToBuilder() {
Duration keepaliveTime = Duration.ofSeconds(1);
Duration keepaliveTimeout = Duration.ofSeconds(2);
ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> channelConfigurator =
new ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder>() {
@Override
public ManagedChannelBuilder apply(ManagedChannelBuilder input) {
throw new UnsupportedOperationException();
}
builder -> {
throw new UnsupportedOperationException();
};
Map<String, ?> directPathServiceConfig = ImmutableMap.of("loadbalancingConfig", "grpclb");

Expand Down Expand Up @@ -266,16 +264,13 @@ public void testWithGCECredentials() throws IOException {
executor.shutdown();

ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> channelConfigurator =
new ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder>() {
@Override
public ManagedChannelBuilder apply(ManagedChannelBuilder channelBuilder) {
if (InstantiatingGrpcChannelProvider.isOnComputeEngine()) {
assertThat(channelBuilder instanceof ComputeEngineChannelBuilder).isTrue();
} else {
assertThat(channelBuilder instanceof ComputeEngineChannelBuilder).isFalse();
}
return channelBuilder;
channelBuilder -> {
if (InstantiatingGrpcChannelProvider.isOnComputeEngine()) {
assertThat(channelBuilder).isInstanceOf(ComputeEngineChannelBuilder.class);
} else {
assertThat(channelBuilder).isNotInstanceOf(ComputeEngineChannelBuilder.class);
}
return channelBuilder;
};

TransportChannelProvider provider =
Expand Down Expand Up @@ -304,13 +299,10 @@ public void testWithNonGCECredentials() throws IOException {
executor.shutdown();

ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> channelConfigurator =
new ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder>() {
@Override
public ManagedChannelBuilder apply(ManagedChannelBuilder channelBuilder) {
// Clients with non-GCE credentials will not attempt DirectPath.
assertThat(channelBuilder instanceof ComputeEngineChannelBuilder).isFalse();
return channelBuilder;
}
channelBuilder -> {
// Clients with non-GCE credentials will not attempt DirectPath.
assertThat(channelBuilder instanceof ComputeEngineChannelBuilder).isFalse();
return channelBuilder;
};

TransportChannelProvider provider =
Expand Down Expand Up @@ -366,13 +358,10 @@ public void testWithNoDirectPathFlagSet() throws IOException {
executor.shutdown();

ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> channelConfigurator =
new ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder>() {
@Override
public ManagedChannelBuilder apply(ManagedChannelBuilder channelBuilder) {
// Clients without setting attemptDirectPath flag will not attempt DirectPath
assertThat(channelBuilder instanceof ComputeEngineChannelBuilder).isFalse();
return channelBuilder;
}
channelBuilder -> {
// Clients without setting attemptDirectPath flag will not attempt DirectPath
assertThat(channelBuilder instanceof ComputeEngineChannelBuilder).isFalse();
return channelBuilder;
};

TransportChannelProvider provider =
Expand Down
2 changes: 2 additions & 0 deletions gax/src/main/java/com/google/api/gax/rpc/StubSettings.java
View file
Open in desktop
Expand Up @@ -176,6 +176,7 @@ public ApiTracerFactory getTracerFactory() {
return tracerFactory;
}

@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("backgroundExecutorProvider", backgroundExecutorProvider)
Expand Down Expand Up @@ -535,6 +536,7 @@ protected static void applyToAllUnaryMethods(

public abstract <B extends StubSettings<B>> StubSettings<B> build() throws IOException;

@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("backgroundExecutorProvider", backgroundExecutorProvider)
Expand Down