feat: add google-c2p dependence to DirectPath #1521
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add tests?
gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Outdated
Show resolved
Hide resolved
gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Outdated
Show resolved
Hide resolved
I am a bit confused about how to write unit test here, as I need to first set the environment variable. After some searching I find it seems not easy to do that in Java. Do you have suggestions here? Or do you think if it is OK that we ignore the unit tests? I have done integration tests before submitting the PR, so the functionalities have been tested. |
Typically this is done through "dependency injection." In fact, the current could have been retrieving directly system environment variables with I'll take care of the merge conflict in |
gax-grpc/build.gradle
Outdated
@@ -18,7 +18,8 @@ dependencies { | |||
libraries['maven.io_grpc_grpc_auth'], | |||
libraries['maven.io_grpc_grpc_netty_shaded'], | |||
libraries['maven.io_grpc_grpc_protobuf'], | |||
libraries['maven.io_grpc_grpc_stub']) | |||
libraries['maven.io_grpc_grpc_stub'], | |||
libraries['maven.io_grpc_grpc_xds']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @ejona86 will adding grpc-xds explode dependencies and the size of gax-grpc library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but that is the design. With added risk, it would be possible to delay adding this as a dependency until users are expected to actually use it. But this would eventually need to happen.
With some effort, the C2P-enabled services could add this dependency instead of gax. So only those services using it would pay the price.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should not add this per my comment above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per my new comment, I think this should be a runtime classpath. Note that you added this to implementation
on line 15. Assuming that it's a runtime dependency, you need to do
runtimeOnly libraries['maven.io_grpc_grpc_xds']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
builder = ComputeEngineChannelBuilder.forTarget("google-c2p:///" + serviceAddress); | ||
} else { | ||
builder = ComputeEngineChannelBuilder.forAddress(serviceAddress, port); | ||
// Set default keepAliveTime and keepAliveTimeout when directpath environment is enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems keepalive should be for google-c2p as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
gax-grpc/build.gradle
Outdated
@@ -18,7 +18,8 @@ dependencies { | |||
libraries['maven.io_grpc_grpc_auth'], | |||
libraries['maven.io_grpc_grpc_netty_shaded'], | |||
libraries['maven.io_grpc_grpc_protobuf'], | |||
libraries['maven.io_grpc_grpc_stub']) | |||
libraries['maven.io_grpc_grpc_stub'], | |||
libraries['maven.io_grpc_grpc_xds']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but that is the design. With added risk, it would be possible to delay adding this as a dependency until users are expected to actually use it. But this would eventually need to happen.
With some effort, the C2P-enabled services could add this dependency instead of gax. So only those services using it would pay the price.
c864c0c
to
8bfcd10
Compare
Thanks for the explanation! I have understood your method and been able to create a unit test, where I when DirectPath XDS is enabled, a URI scheme Also notice that in my manual test it failed because |
8f187f1
to
77e81d7
Compare
@@ -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']}", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, thanks!
gax-grpc/build.gradle
Outdated
@@ -18,7 +18,8 @@ dependencies { | |||
libraries['maven.io_grpc_grpc_auth'], | |||
libraries['maven.io_grpc_grpc_netty_shaded'], | |||
libraries['maven.io_grpc_grpc_protobuf'], | |||
libraries['maven.io_grpc_grpc_stub']) | |||
libraries['maven.io_grpc_grpc_stub'], | |||
libraries['maven.io_grpc_grpc_xds']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should not add this per my comment above?
@@ -390,6 +407,29 @@ public ManagedChannelBuilder apply(ManagedChannelBuilder channelBuilder) { | |||
provider.getTransportChannel().shutdownNow(); | |||
} | |||
|
|||
@Test | |||
public void testWithDirectPathXds() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for trying. I saw this test is failing, and I think I know why. In order to test the section of the interest, we should satisfy the following constraints:
if (isDirectPathEnabled(serviceAddress)
&& isNonDefaultServiceAccountAllowed()
&& isOnComputeEngine()) {
The problem is that, isOnComputeEngine()
is not controllable. Certainly, this class is not very testable. For making this testable, we should further refactor the class so that the information about whether it's on GCE is given from another provider (like an env provider).
But at this point, I don't want to make your life too difficult. Let's forget adding this and the setEnvProvider
/activeEndpoint
stuff to maintain simple code. Sorry, I appreciate your time you spent to explore this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks! I will remove the unit test then.
I pushed some changes, so be sure to pull before working on this. |
@chanseokoh Hey Chanseok, can you also help to merge this PR? Sorry I do not have the access. Thanks! |
Sure, just merged it. |
DirectPath is migrating to Traffic Director, and we want to setup E2E tests based on TD for Bigtable and Spanner. This PR: 1. Update grpc-go version to 1.41.0, which has RING_HASH LB policy support; 2. Add the google-c2p resolver dependence to google-api-go-client; 3. Add an environment variable GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS to enable using DirectPath over TD; Corresponding PR in gax-java: googleapis/gax-java#1521. Also note that currently there is a bug in grpc-go, and fix is grpc/grpc-go#4889. I have verified that with this fix Bigtable E2E test over DirectPath Traffic Director can pass. This PR has been merged to grpc-go and will release in grpc-go 1.42.0, which will be the minimal version for supporting `google-c2p` URI scheme.
DirectPath is migrating to Traffic Director, and we want to setup E2E tests based on TD for Bigtable and Spanner. This PR:
GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS
to enable using DirectPath over TD;