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

clientv3: use grpc balancer #5583

Merged
merged 5 commits into from
Jun 8, 2016

Conversation

heyitsanthony
Copy link
Contributor

No description provided.

@gyuho
Copy link
Contributor

gyuho commented Jun 8, 2016

Maybe rebase the grpc dep with your fix grpc/grpc-go#717? Just got merged.

"google.golang.org/grpc"
)

type balancer struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

randomizedBalancer?

Copy link
Contributor

Choose a reason for hiding this comment

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

any reason for not using the default roundrobin balancer provided by grpc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using the grpc roundrobin balancer would just as complicated (or more so) since it needs resolver and watcher interface implementations

Copy link
Contributor

Choose a reason for hiding this comment

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

ok.

@xiang90
Copy link
Contributor

xiang90 commented Jun 8, 2016

LGTM after fixing tests.

@gyuho
Copy link
Contributor

gyuho commented Jun 8, 2016

So rpctypes.ErrConnClosed == grpc.ErrClientConnClosing, now?

LGTM.

/cc @purpleidea @siddontang regarding the issue #5495.

@heyitsanthony
Copy link
Contributor Author

@gyuho yes. It probably didn't belong in rpctypes in the first place since the error isn't transmitted from the server.

@heyitsanthony heyitsanthony force-pushed the grpc-nuke-waitstate branch 4 times, most recently from deba127 to 70f642d Compare June 8, 2016 07:32
Tests need to disconnect the network connection for the client to check
reconnection paths but closing a grpc connection closes the logical connection.
To disconnect the client, instead have a bridge between the server and
the client which can monitor and reset connections.
@heyitsanthony heyitsanthony force-pushed the grpc-nuke-waitstate branch 2 times, most recently from 0be40af to 6b13c7f Compare June 8, 2016 08:02
@heyitsanthony heyitsanthony force-pushed the grpc-nuke-waitstate branch 3 times, most recently from 782283b to 810f11c Compare June 8, 2016 16:05
@heyitsanthony heyitsanthony merged commit ff2b24a into etcd-io:master Jun 8, 2016
@heyitsanthony heyitsanthony deleted the grpc-nuke-waitstate branch June 8, 2016 17:58
@purpleidea
Copy link
Contributor

There are virtually no commit messages or docs associated with this that I can find. Is there a short summary somewhere that we can read to learn more? Thanks!

@xiang90
Copy link
Contributor

xiang90 commented Jun 9, 2016

@purpleidea
Copy link
Contributor

On Wed, Jun 8, 2016 at 11:09 PM, Xiang Li notifications@github.com wrote:

@purpleidea https://github.com/purpleidea
https://godoc.org/google.golang.org/grpc#Balancer.

I just don't understand what the implications for etcd are, sorry.

@heyitsanthony
Copy link
Contributor Author

@purpleidea the motivation was so that go get github.com/coreos/etcd/clientv3 would work/compile again after a grpc API change. The functional difference is network reconnection logic is being handled by grpc now so the client is much simpler. grpc will be able to dynamically configure the client's endpoints as it's running so it won't be necessary to tear down the client to change endpoints

@purpleidea
Copy link
Contributor

@heyitsanthony Gahhh! Sounds like #5491 ... I wish I knew this was coming, great feature, but I've just spent three or so days fixing my code so that it can handle this scenario! Oh well glad to know this is coming.

I have a bunch of code that depends on it if you'd like to test your new API for this before it goes mainstream. I hope it includes dealing with the change when an endpoint crashes or is fenced somehow.

As an aside: where is the best place to learn about all these new and planned changes. Is there a mailing list where it's discussed or is it internal only chats? It would probably save me and other developers time to know what's going on.

Cheers!

@heyitsanthony
Copy link
Contributor Author

@purpleidea https://github.com/coreos/etcd/milestones has the planning. It's difficult to capture everything to the last detail though.

@purpleidea
Copy link
Contributor

On Wed, Jun 8, 2016 at 11:46 PM, Anthony Romano notifications@github.com
wrote:

@purpleidea https://github.com/purpleidea
https://github.com/coreos/etcd/milestones has the planning. It's
difficult to capture everything to the last detail though.

This is somewhat useful, thanks! I am interested in the big picture ideas,
so if there are ghangouts or similar where discussions are happening and
you feel like including me, please do! Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants