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

v0.60.0 pulls in 300kLOC of additional dependencies #1283

Closed
tamird opened this issue Nov 3, 2021 · 9 comments
Closed

v0.60.0 pulls in 300kLOC of additional dependencies #1283

tamird opened this issue Nov 3, 2021 · 9 comments
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@tamird
Copy link

tamird commented Nov 3, 2021

aa0f0be added a dependency on google.golang.org/grpc/xds which pulls in a lot of code.

It should be possible to upgrade past v0.60.0 without taking on all this bloat.

@tamird tamird added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Nov 3, 2021
@codyoss
Copy link
Member

codyoss commented Nov 3, 2021

@mohanli-ml is there anything we can do here to lower the complexity of dependencies?

@codyoss
Copy link
Member

codyoss commented Nov 3, 2021

Can this resolver registration be moved to the site of an actual client?, or even better the program making use of our client? It seems like the bulk of the deps are related to pulling in a lot of the envoy project.

@mohanli-ml
Copy link
Contributor

@menghanl @dfawley Any suggestions here?

@menghanl
Copy link

menghanl commented Nov 3, 2021

This import _ "google.golang.org/grpc/xds/googledirectpath" adds the extra dependencies.
(aa0f0be#diff-8a842d9ae06c25d12d6cf1e82d5057be6f81be7a2d75f82cc45aa28207f13023R16)

Direct path depends on xDS. To enable direct path, the library will need to pull in the xDS dependencies (including the xDS protos from the envoy project).

Is it OK for cloud libraries to not have direct path capability by default, and ask users to enable direct path?
If the answer is yes, we can delete that import and ask the users to add it.
But note that it's a code change that they will need to make.

@mohanli-ml
Copy link
Contributor

Also if the decision is to move the xds dependency from the google-api-go-client repo to service repo (like bigtable-go, spanner-go), I want to know if we should also do this for other languages?

@codyoss
Copy link
Member

codyoss commented Nov 4, 2021

@mohanli-ml I think that it should at least be moved there. But I wonder if a better pattern would be to, through docs, tell users to add the registration import themselves. Then all users of spanner and bigtable don't have to pay the cost of these deps. I am not sure what the semantics look like in other languages, but if something similar could be done there I don't think that would be a bad idea.

@dfawley
Copy link

dfawley commented Nov 4, 2021

It seems a little strange to me that client libraries would have code that requires an import (using the "google-c2p" resolver scheme), but doesn't actually do the import itself. And, it doesn't detect this scenario and fall back, either.

Maybe, instead of (or in addition to?) checking the gRPC version, it should look up the resolver and use it only if it is found? I.e. [grpc/]resolver.Get("google-c2p") != nil. Otherwise, fall back to non-directpath.

@mohanli-ml
Copy link
Contributor

I will have another PR to remove the google-c2p dependence in google-api-go-client after the PR to add them to bigtable/spanner gets merged.

codyoss pushed a commit to googleapis/google-cloud-go that referenced this issue Dec 21, 2021
…nner (#5090)

According to discussion in googleapis/google-api-go-client#1283, we plan to move the google-c2p dependence to bigtable/spanner, which are services that will use DirectPath.


feat(bigtable): add google-c2p dependence

feat(spanner): add google-c2p dependence
codyoss pushed a commit that referenced this issue Dec 21, 2021
Remove google-c2p dependency to the entire google-api-go-client. Instead, services using DirectPath should add the dependency by itself, like googleapis/google-cloud-go#5090.

Issue: #1283
BrennaEpp pushed a commit to BrennaEpp/google-cloud-go that referenced this issue Dec 23, 2021
…nner (googleapis#5090)

According to discussion in googleapis/google-api-go-client#1283, we plan to move the google-c2p dependence to bigtable/spanner, which are services that will use DirectPath.


feat(bigtable): add google-c2p dependence

feat(spanner): add google-c2p dependence
@codyoss
Copy link
Member

codyoss commented Jan 10, 2022

This should have been addressed in a recent release. Closing.

@codyoss codyoss closed this as completed Jan 10, 2022
github-actions bot pushed a commit to gnoliyil/fuchsia that referenced this issue Jan 11, 2022
googleapis/google-api-go-client#1283 is fixed.

Change-Id: Iaf1db4891043508ff41347e7c6fcfe341423c773
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/629341
Reviewed-by: Bruno Dal Bo <brunodalbo@google.com>
Fuchsia-Auto-Submit: Tamir Duberstein <tamird@google.com>
Commit-Queue: Auto-Submit <auto-submit@fuchsia-infra.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

5 participants