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

all: remove last uses of gtransport.Dial #1777

Closed
broady opened this issue Feb 12, 2020 · 10 comments
Closed

all: remove last uses of gtransport.Dial #1777

broady opened this issue Feb 12, 2020 · 10 comments
Assignees
@broady @codyoss @noahdietz @tbpg
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

@broady
Copy link
Contributor

broady commented Feb 12, 2020 

$ git grep transport.Dial | grep -v DialPool | grep -v test | cut -d: -f1 | sort | uniq
asset/v1beta1/asset_client.go
automl/apiv1/auto_ml_client.go
automl/apiv1beta1/auto_ml_client.go
automl/apiv1beta1/prediction_client.go
automl/apiv1/prediction_client.go
bigquery/datatransfer/apiv1/data_source_client.go
cloudbuild/apiv1/cloud_build_client.go
containeranalysis/apiv1/container_analysis_client.go
container/apiv1/cluster_manager_client.go
grafeas/apiv1/grafeas_client.go
iam/admin/apiv1/iam_client.go
pubsub/apiv1/publisher_client.go
pubsub/apiv1/subscriber_client.go
talent/apiv4beta1/application_client.go
talent/apiv4beta1/company_client.go
talent/apiv4beta1/completion_client.go
talent/apiv4beta1/event_client.go
talent/apiv4beta1/job_client.go
talent/apiv4beta1/profile_client.go
talent/apiv4beta1/tenant_client.go

@shollyman re: bigquery/datatransfer/apiv1/data_source_client: "we can probably just remove it"
@broady broady added triage me I really want to be triaged. 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. type: cleanup An internal cleanup or hygiene concern. and removed triage me I really want to be triaged. labels Feb 12, 2020
@broady broady assigned noahdietz, tbpg and broady and unassigned noahdietz and tbpg Feb 12, 2020
@noahdietz
Copy link
Contributor 

automl/apiv1/auto_ml_client.go
automl/apiv1beta1/auto_ml_client.go
automl/apiv1beta1/prediction_client.go
automl/apiv1/prediction_client.go
pubsub/apiv1/publisher_client.go
pubsub/apiv1/subscriber_client.go
talent/apiv4beta1/application_client.go
talent/apiv4beta1/company_client.go
talent/apiv4beta1/completion_client.go
talent/apiv4beta1/event_client.go
talent/apiv4beta1/job_client.go
talent/apiv4beta1/profile_client.go
talent/apiv4beta1/tenant_client.go

These will be handled by moving to microgenerator.

@noahdietz
Copy link
Contributor

asset/v1beta1/asset_client.go

This client should be removed, it doesn't conform to apiv prefixing and was added accidentally at some point.

@broady
Copy link
Contributor Author

broady commented Feb 12, 2020

Any ETA on that, @noahdietz? Primarily concerned about pubsub/apiv1 which is used by hand-written pubsub client, which uses pooling by default.

@broady
Copy link
Contributor Author

broady commented Feb 12, 2020

Blocks googleapis/google-api-go-client#441

@noahdietz
Copy link
Contributor

pubsub and automl have CLs open internally to configure their protos. They are under review by the API teams. I can push on them.

The talent microgen migration results in a client lib breaking change and the API team requested a different client lib breaking change in response to the first one, which requires work on the generator. We're close to committing to it, but need to sync up with the API team first.

@noahdietz
Copy link
Contributor 

cloudbuild/apiv1/cloud_build_client.go
containeranalysis/apiv1/container_analysis_client.go
container/apiv1/cluster_manager_client.go
grafeas/apiv1/grafeas_client.go
iam/admin/apiv1/iam_client.go

These aren't generated anymore and require manual work. I have little context on why they aren't generated anymore.

@broady
Copy link
Contributor Author

broady commented Feb 27, 2020

Current list as of Feb 26, 2020:

Moving to new generator soon:

automl/apiv1/auto_ml_client.go
automl/apiv1beta1/auto_ml_client.go
automl/apiv1beta1/prediction_client.go
automl/apiv1/prediction_client.go

Fixed soon due to migration to microgenerator:

pubsub/apiv1/publisher_client.go
pubsub/apiv1/subscriber_client.go

Stale/not generated anymore (per comment above):

cloudbuild/apiv1/cloud_build_client.go
containeranalysis/apiv1/container_analysis_client.go
container/apiv1/cluster_manager_client.go
grafeas/apiv1/grafeas_client.go
iam/admin/apiv1/iam_client.go

@shollyman: can we really remove this? It doesn't have "alpha" or "beta" in the path and there's no doc comment saying it's experimental.

bigquery/datatransfer/apiv1/data_source_client.go

@broady
Copy link
Contributor Author

broady commented Mar 2, 2020

Current list as of March 3, 2020:

automl/apiv1/auto_ml_client.go
automl/apiv1/prediction_client.go
automl/apiv1beta1/auto_ml_client.go
automl/apiv1beta1/prediction_client.go
bigquery/datatransfer/apiv1/data_source_client.go
cloudbuild/apiv1/cloud_build_client.go
container/apiv1/cluster_manager_client.go
containeranalysis/apiv1/container_analysis_client.go
grafeas/apiv1/grafeas_client.go
iam/admin/apiv1/iam_client.go

Pub/Sub was the last API blocking CL 50573

gopherbot pushed a commit to googleapis/google-api-go-client that referenced this issue Mar 3, 2020
@broady @codyoss
transport/grpc: WithGRPCConnectionPool is a no-op for some APIs
fe7fc1f 
Connection pooling is now done via transport/grpc.DialPool and is no
longer supported with transport/grpc.Dial.

See googleapis/google-cloud-go#1777 for
the list of packages that will no longer support gRPC connection
pooling after this change is merged.

Fixes #441

Change-Id: I8e05af53940d6e0af3c2acce9f81b49eaa78bd54
Reviewed-on: https://code-review.googlesource.com/c/google-api-go-client/+/50573
Reviewed-by: kokoro <[email protected]>
Reviewed-by: Cody Oss <[email protected]>
gopherbot pushed a commit that referenced this issue Mar 11, 2020
@olavloite
spanner: Use a connection pool
b63d8a6 
The Spanner client library did round-robin connection pooling manually.
This changes it to use the new connection pooling in gRPC.

This can BREAK user code in the following specific case:
* The user specified a value for NumChannels in ClientConfig
* The user specified a different value for WithGRPCConnectionPool

Example:
The following code would work prior to this change, while it will
return an error after this change.

client, err := NewClientWithConfig(
	context.Background(),
	"projects/p/instances/i/databases/d",
	ClientConfig{NumChannels: 8},
	option.WithGRPCConnectionPool(16),
)

The example code does however represent a mis-configuration that would
break session-channel affiliation, and also would not make any sense.
Instead of silently ignoring the mis-configuration and use one of the
configured values, the client library now returns an error.

This change DEPRECATES ClientConfig.NumChannels.

Updates #1777.

Change-Id: I14424296cb41f012770186580e9466fec0e5b073
Reviewed-on: https://code-review.googlesource.com/c/gocloud/+/49874
Reviewed-by: kokoro <[email protected]>
Reviewed-by: Hengfeng Li <[email protected]>
@codyoss
Copy link
Member

codyoss commented Apr 28, 2020

Is there more to do here still?

@broady
Copy link
Contributor Author

broady commented Apr 28, 2020 •
edited
Loading

Still remaining:

bigquery/datatransfer/apiv1/data_source_client.go
cloudbuild/apiv1/cloud_build_client.go
containeranalysis/apiv1/container_analysis_client.go
grafeas/apiv1/grafeas_client.go
iam/admin/apiv1/iam_client.go

@JustinBeckwith JustinBeckwith removed the type: cleanup An internal cleanup or hygiene concern. label May 14, 2020
@codyoss codyoss self-assigned this May 21, 2020
codyoss added a commit to googleapis/go-genproto that referenced this issue May 28, 2020
@codyoss
all: update .pb.go files that are no longer generated
e762faf 
This is required to fully remove refs to gransport.Dial in gocloud.

Updates: googleapis/google-cloud-go#1777
@codyoss codyoss mentioned this issue May 28, 2020
Merged
codyoss added a commit to googleapis/go-genproto that referenced this issue May 28, 2020
@codyoss
all: update .pb.go files that are no longer generated (#376)
705c0b3 
This is required to fully remove refs to gransport.Dial in gocloud.

Updates: googleapis/google-cloud-go#1777
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@broady broady

@codyoss codyoss

@noahdietz noahdietz

@tbpg tbpg

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
Milestone
No milestone
Development

No branches or pull requests
5 participants
@broady @JustinBeckwith @codyoss @noahdietz @tbpg