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

chore: update temporary entries for cloud.google.com/go/spanner in go.mod to public versions supporting PG feature #265

Merged
merged 3 commits into from Mar 7, 2022

Conversation

rahul2393
Copy link
Contributor

@rahul2393 rahul2393 commented Mar 4, 2022

Fixes #<issue_number_goes_here>

  • Tests pass
  • Appropriate changes to README are included in PR

This PR also adds naming module to repository.

What is the motivation behind this?

We need this because of a 'dependency hell' situation:

  1. grpc-go makes breaking changes between minor releases of their module.
  2. tidb v1.1.0-beta depends on a grpc/naming API which was removed in v1.30. We depend on etcd v0.5.0-alpha indirectly via tidb v1.1.0-beta.

This situation would be solved by bumping tidb to version which depends on etcd v3.6.0+, which does not make use of the removed grpc-go API.

This module is used to solve this dependency hell, by adding the google.golang.org/grpc/naming package into version v1.44 of grpc-go.

It is the smallest patch that can be applied to solve this particular 'dependency hell' issue with grpc-go. It will not fix other issues when packages depend on other, newer packages.

When can it be removed?

This must be removed when tidb does not need grpc/naming.

Copy link
Collaborator

@bharadwaj-aditya bharadwaj-aditya left a comment

Choose a reason for hiding this comment

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

LGTM

*
*/

package naming
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a comment linking where this snippet has been taken from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think we need to add here too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense to add a comment here too, just to make it clear that this is going to be a copy of the source and not to add any custom code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!! Github is not showing Outdated for some reasons.


// Package naming defines the naming API and related data structures for gRPC.
//
// This package is deprecated: please use package resolver instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add a comment as to where this snippet has been taken from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!! Github is not showing Outdated for some reasons.

@Deep1998
Copy link
Collaborator

Deep1998 commented Mar 7, 2022

LGTM, thanks for this fix. Please update the PR description as well though. You can simply copy paste the readme contents.

@bharadwaj-aditya bharadwaj-aditya merged commit e22acad into GoogleCloudPlatform:master Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants