Skip to content

Conversation

gouthamve
Copy link
Contributor

@bboreham
Copy link
Collaborator

Are there any notable bug fixes or feature changes in this version?

Signed-off-by: Goutham Veeramachaneni <[email protected]>
@gouthamve gouthamve force-pushed the update-kube-resolver branch from cd0bf40 to cd2d48e Compare November 19, 2018 13:03
@gouthamve
Copy link
Contributor Author

Nope, but it uses the new gRPC API for loadblancing. It's just the updated version of the previous library.

@bboreham
Copy link
Collaborator

Review might go faster if you say what is going on in the 80-odd lines of changes.

Signed-off-by: Goutham Veeramachaneni <[email protected]>
@gouthamve
Copy link
Contributor Author

Previously, we needed to pass in a resolver as an option. And the resolver used to do both resolution and loadbalancing, afaik.

Now, the API changed to "registering" resolvers. i.e, mapping scheme to resolvers, for example kubernetes:// --> kuberesolver, dns:// --> dns resolver, etc. And then specifying the load-balancing policy via the resolution mechanism or manually or via an grpclb, etc.

See: https://github.com/grpc/proposal/blob/master/L9-go-resolver-balancer-API.md

In this PR, I modified our client creation to use the new API through kuberesolver v2. It was fairly straightforward, needed to remove the additional resolver dial option and needed to specify the loadbalancing.

@tomwilkie
Copy link
Contributor

I was also hoping that kuberesolver v2 would fix sercand/kuberesolver#4, but it doesn't appear to.

Copy link
Collaborator

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Thanks!

@bboreham bboreham merged commit 526f1ec into weaveworks:master Nov 20, 2018
@bboreham
Copy link
Collaborator

for example kubernetes:// --> kuberesolver, dns:// --> dns resolver, etc.

According to https://github.com/grpc/grpc-go/blob/v1.19.0/clientconn.go#L125 those should have three slashes. Did you test this code?

@gouthamve
Copy link
Contributor Author

Oh right, we ended up using dns:/// which worked. Though let me update the code to make it :///

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.

3 participants