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

*: refactor clientv3 balancer, upgrade gRPC to v1.7.2 #8840

Merged
merged 5 commits into from
Nov 10, 2017
Merged

*: refactor clientv3 balancer, upgrade gRPC to v1.7.2 #8840

merged 5 commits into from
Nov 10, 2017

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Nov 8, 2017 •
edited
Loading

If we want to notify only healthy addresses, we need a way to look up an endpoint is healthy or not, which is tracked in healthBalancer. This combines healthBalancer and simpleBalancer. Plus, they already have many redundant fields (e.g. eps, hostPort2eps, etc.).

Fix #8828.

@gyuho gyuho added the WIP label Nov 8, 2017
@gyuho gyuho changed the title clientv3: remove "healthBalancer" clientv3: combine "healthBalancer" into "simpleBalancer" Nov 8, 2017
@gyuho gyuho force-pushed the health-balancer branch 5 times, most recently from b382fd9 to b08eac4 Compare November 8, 2017 18:18
@gyuho gyuho removed the WIP label Nov 8, 2017
@gyuho gyuho requested a review from xiang90 November 8, 2017 21:30
@gyuho gyuho added the WIP label Nov 8, 2017
@gyuho gyuho force-pushed the health-balancer branch 3 times, most recently from f4f7817 to eef8ecf Compare November 8, 2017 23:05
@gyuho gyuho mentioned this pull request Nov 8, 2017
Merged
@xiang90
Copy link
Contributor

xiang90 commented Nov 9, 2017

let us focus on existing test. not #8841

@xiang90
Copy link
Contributor

xiang90 commented Nov 9, 2017

can we remove the new test out of this?

@gyuho gyuho closed this Nov 9, 2017
@gyuho gyuho deleted the health-balancer branch November 9, 2017 18:21
@xiang90
Copy link
Contributor

xiang90 commented Nov 9, 2017

@gyuho why this is closed?

@gyuho gyuho restored the health-balancer branch November 9, 2017 18:27
@gyuho gyuho reopened this Nov 9, 2017
@gyuho gyuho force-pushed the health-balancer branch 3 times, most recently from 004ca1a to 345e89d Compare November 9, 2017 19:32
@gyuho gyuho changed the title clientv3: combine "healthBalancer" into "simpleBalancer" clientv3: refactor balancer, upgrade gRPC to v1.7.2 Nov 9, 2017
@gyuho gyuho changed the title clientv3: refactor balancer, upgrade gRPC to v1.7.2 *: refactor clientv3 balancer, upgrade gRPC to v1.7.2 Nov 9, 2017
@gyuho gyuho force-pushed the health-balancer branch 2 times, most recently from f85a0db to 44ad0ae Compare November 9, 2017 20:54
@gyuho gyuho force-pushed the health-balancer branch 2 times, most recently from c2deb20 to 936229e Compare November 9, 2017 22:37
@gyuho gyuho removed the WIP label Nov 9, 2017
@gyuho
Copy link
Contributor Author

gyuho commented Nov 9, 2017 •
edited
Loading

I ran test suites several times in other VMs: they all pass, except coverage tests...

https://github.com/coreos/etcd/blob/75c7e62dc7ce9a4cc2c7ace8f596e8384d779d36/test#L189-L194

=== RUN   TestCtlV2Backup
--- FAIL: TestCtlV2Backup (2.64s)
	ctl_v2_test.go:290: unexpected output (got lines [], line count 13)

Do we want to block on this?

@xiang90
Copy link
Contributor

xiang90 commented Nov 9, 2017

No. The coverage thing is a separate issue I believe.

I am going to review the whole thing tomorrow, and hopefully get this merged.

xiang90
xiang90 reviewed Nov 10, 2017
View reviewed changes
clientv3/retry.go Outdated
@@ -62,6 +62,11 @@ func (c *Client) newRetryWrapper(isStop retryStopErrFunc) retryRPCFunc {
if logger.V(4) {
logger.Infof("clientv3/retry: error %q on pinned endpoint %q", err.Error(), pinned)
}
// retry when initial connection has not been established
// grpc/grpc-go >v1.7.x (balancer v1 wrapper)
if rpctypes.ErrorDesc(err) == "there is no connection available" {
Copy link
Contributor

Choose a reason for hiding this comment

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

make the error msg as a const?

@gyuho gyuho force-pushed the health-balancer branch 2 times, most recently from 097252d to 4985bd0 Compare November 10, 2017 21:47
xiang90
xiang90 reviewed Nov 10, 2017
View reviewed changes
clientv3/health_balancer_test.go Outdated
@@ -66,8 +66,8 @@ func TestBalancerGetUnblocking(t *testing.T) {
}

down1(errors.New("error"))
if addrs := <-sb.Notify(); len(addrs) != len(endpoints) {
t.Errorf("closing the only connection should triggered balancer to send the all endpoints via Notify chan so that we can establish a connection")
if addrs := <-sb.Notify(); len(addrs) != 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

len(addrs) != len(endpoints) - 1 // we call down on one endpoint

xiang90
xiang90 reviewed Nov 10, 2017
View reviewed changes
clientv3/health_balancer_test.go Outdated
@@ -121,8 +121,8 @@ func TestBalancerGetBlocking(t *testing.T) {
}

down1(errors.New("error"))
if addrs := <-sb.Notify(); len(addrs) != len(endpoints) {
t.Errorf("closing the only connection should triggered balancer to send the all endpoints via Notify chan so that we can establish a connection")
if addrs := <-sb.Notify(); len(addrs) != 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

len(addrs) != len(endpoints) - 1 // we call down on one endpoint

@gyuho gyuho force-pushed the health-balancer branch 2 times, most recently from ca99730 to 3ca6d9e Compare November 10, 2017 21:55
@xiang90
Copy link
Contributor

xiang90 commented Nov 10, 2017

lgtm if CI is reliably passing.

@gyuho gyuho force-pushed the health-balancer branch 4 times, most recently from c417ed1 to 335d8a6 Compare November 10, 2017 22:56
@gyuho gyuho merged commit b64c1bf into etcd-io:master Nov 10, 2017
@gyuho gyuho deleted the health-balancer branch November 10, 2017 23:41
@jpbetz jpbetz mentioned this pull request Nov 21, 2017
Closed
@xarses xarses mentioned this pull request Jun 15, 2018
Closed
@xarses
Copy link

xarses commented Jun 15, 2018

I'm wondering if the gRPC proxy uses this enough to put in front of older clients so that I can get this behavior until I can get them on updated to use this directly.

@gyuho
Copy link
Contributor Author

gyuho commented Jun 18, 2018

Copying from slack conversation:

gRPC Proxy is implemented as an etcd client wrapper, so if the gRPC Proxy is built with latest v3.2 release, it should support failover. But, "client talking to gRPC Proxy" may not work as well as regular client talking to core etcd server, because we do not support keepalive ping in gRPC proxy layer yet. Need more tests around gRPC proxy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiang90 xiang90 xiang90 left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gyuho @xiang90 @xarses