-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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(transport): add google-c2p dependence to DirectPath #1260
Conversation
@mohanli-ml This library can no longer update the gRPC dependency as we stay compatible with 1.11. We are hoping to up our supported version early next year. But for now if there are new features needed in gRPC we need to try to get them backported or wait. |
cc @dfawley This is the feature I had flagged in earlier discussions. |
Is it possible for customers that want directpath to declare |
Monday morning ping. My question above was for @mohanli-ml, to see if it's feasible to have customers do this themselves (use grpc-go v1.41+), instead of doing it globally. |
Thanks Doug, I have the same questions too. @codyoss I just tried to move the xds registry |
@mohanli-ml There is no way for us to optionally declare a dependency at a higher version is some situations and lower in others. Build tags don't help in this type of situation either. |
The question is about two things:
If the answers are "no, yes", then we don't need to pin v1.41 here; users can do it instead. Otherwise, we have to figure something out. |
Yes. To make this work we need grpc/grpc-go#4889, so I think the minimal grpc-go version is 1.42.
I think the answer is No, as explained by Cody: "If I took a dep bump to 41 in any client library today that means no customer could deploy to cloud functions /w Go and a modern version of the client library. We need to try not to move passed the types of environments that our serverless teams support in GA today." |
This PR doesn't seem to create any new exported symbols, so I believe this statement is false. We would simply tell users "if you want to use directpath, please make sure you are using grpc-go v1.42+", rather than enforcing that requirement here.
This statement by Cody is about changing the default requirement here. Or are you implying it's required for us to support directpath for cloud functions users before CF rolls out a less-ancient version of Go to their portfolio? Note the cloud functions does have "preview" support for 1.16, so they could run in that environment. If directpath is also "preview" at this point, then that seems not unreasonable to me. |
Thanks Doug! After investigation I think we should be able to enable XDS support in google-api-go-client without enforcing grpc version 1.42+. @codyoss I have updated the PR, PTAL. Basically changes are:
I believe in this way CBT probers and E2E tests over Traffic Director can be unblocked. For example, we can do bigtable tests like this:
|
The change to transport/grpc/dial_enablexds.go in this PR causes a dependency on |
Yes, adding xds support will explode dependencies and this size of google-api-go-client library, unfortunately. We have the same issue in other languages (for example, java googleapis/gax-java#1521). I thought the dependencies are protected by the build tag, but looks like even if these codes are not compiled, they are still pulled? |
Yes, the Go module system can't know which build tags will be used, so it pulls all possible combinations. |
Filed #1283. |
Since googleapis/google-api-go-client#1260 we need to start allowing the service config to be pulled. cc @mohanli-ml
DirectPath is migrating to Traffic Director, and we want to setup E2E tests based on TD for Bigtable and Spanner. This PR:
Corresponding PR in gax-java: feat: add google-c2p dependence to DirectPath 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.