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

clientv3: Replace balancer with upstream grpc solution #12671

Merged

ptabor merged 1 commit into etcd-io:master from ptabor:20210207-grpc-del-balancer

Feb 17, 2021

Contributor

ptabor commented Feb 7, 2021

Replace custom balancer logic in clientv3 with grpc infrastructure:

custom resolver + round_robin load balancer.

This commit significantly reduces volume of custom code
in etcd client v3, while preserving full existing functionality.

ptabor added the area/clientv3 label

ptabor requested a review from jpbetz

February 7, 2021 22:55

ptabor force-pushed the 20210207-grpc-del-balancer branch from f6e5e5d to 69cf181 Compare

February 7, 2021 23:04


          Replace client/v3/balancer with standard components: resolver + round…

0b75fed

…_robin LB

This commit significantly reduces volume of custom code
in etcd client v3, while preserving full existing functionality.

ptabor force-pushed the 20210207-grpc-del-balancer branch from 69cf181 to 0b75fed Compare

February 8, 2021 17:50

ptabor mentioned this pull request

grpc bump: Implement master/client/v3/naming new endpointManager & resolver #12652

Closed

ptabor requested a review from jingyih

February 10, 2021 11:16

Contributor Author

ptabor commented Feb 11, 2021

@jpbetz @jingyih @gyuho - this is pretty substantial change. Please let me know if I have not missed any 'special' important properties of the (previous) balancer implementation.

ptabor mentioned this pull request

pkg/types: Support Unix sockets in NewURLS #12469

Merged

Contributor

jpbetz commented Feb 16, 2021

LGTM

Contributor

jpbetz commented Feb 16, 2021

@jingyih any thoughts?

ptabor merged commit b67ed4e into etcd-io:master

ptabor deleted the 20210207-grpc-del-balancer branch

February 17, 2021 21:45

gyuho reviewed

View reviewed changes

client/v3/client.go

    
              	if creds != nil {

              		opts = append(opts, grpc.WithTransportCredentials(creds))

              	} else {

              		opts = append(opts, grpc.WithInsecure())

              	}

              	opts = append(opts, grpc.WithContextDialer(dialer))

              	grpc.WithDisableRetry()

Contributor

gyuho Feb 19, 2021

Can we add a comment on this? So, now gRPC enables retry by default?

jqmichael Mar 6, 2021 •

edited

So, now gRPC enables retry by default?

I don't think so.

	// Retry is set if retry is explicitly enabled via "GRPC_GO_RETRY=on".
	Retry = strings.EqualFold(os.Getenv(retryStr), "on")

https://github.com/grpc/grpc-go/blame/61f0b5fa7c1c375e2bbf29f2f5f8610d2b5ba956/internal/envconfig/envconfig.go#L34

Contributor Author

ptabor Mar 6, 2021

I removed this in follow up PR:

4a1c245#diff-2bbaa3c83234888775aabf7697076e8035451045c479938e03ebb5c929132679L209

In general we should replace clientv3 retry interceptors with RetryPolicy.

gyuho reviewed

View reviewed changes

client/v3/client.go Show resolved Hide resolved

gyuho reviewed

View reviewed changes

client/v3/client.go Show resolved Hide resolved

gyuho reviewed

View reviewed changes

client/v3/internal/endpoint/endpoint.go Show resolved Hide resolved

gyuho reviewed

View reviewed changes

client/v3/internal/endpoint/endpoint.go Show resolved Hide resolved

gyuho reviewed

View reviewed changes

client/v3/internal/endpoint/endpoint.go Show resolved Hide resolved

gyuho reviewed

View reviewed changes

Contributor

gyuho left a comment

Thanks for clean migration!

Can we please add more comments and address other comments here?

ptabor added a commit to ptabor/etcd that referenced this pull request


          clientv3: PS: Replace balancer with upstream grpc solution

cb1a62c

Addresses comments from: etcd-io#12671 (review)

ptabor mentioned this pull request

clientv3: PS: Replace balancer with upstream grpc solution #12706

Merged

ptabor added a commit to ptabor/etcd that referenced this pull request


          clientv3: PS: Replace balancer with upstream grpc solution

4f94bb0

Addresses comments from: etcd-io#12671 (review)

ptabor added a commit to ptabor/etcd that referenced this pull request


          clientv3: PS: Replace balancer with upstream grpc solution

377f680

Addresses comments from: etcd-io#12671 (review)

ptabor added a commit to ptabor/etcd that referenced this pull request


          clientv3: PS: Replace balancer with upstream grpc solution

5720aee

Addresses comments from: etcd-io#12671 (review)

ptabor added a commit to ptabor/etcd that referenced this pull request


          clientv3: PS: Replace balancer with upstream grpc solution

7901bfc

Addresses comments from: etcd-io#12671 (review)

ptabor added a commit to ptabor/etcd that referenced this pull request


          clientv3: PS: Replace balancer with upstream grpc solution

43a39ec

Addresses comments from: etcd-io#12671 (review)

ptabor added a commit to ptabor/etcd that referenced this pull request


          clientv3: PS: Replace balancer with upstream grpc solution

13a271e

Addresses comments from: etcd-io#12671 (review)

ptabor added a commit to ptabor/etcd that referenced this pull request


          clientv3: PS: Replace balancer with upstream grpc solution

4a1c245

Addresses comments from: etcd-io#12671 (review)

Contributor

gyuho commented Mar 6, 2021

ptabor mentioned this pull request

apiserver: disable info level gRPC logging kubernetes/kubernetes#84061

Closed

ptabor mentioned this pull request

Improve make-mirror endpoint handling #10422

Closed

ptabor mentioned this pull request

Silence grpc connection log noise. kubernetes/kubernetes#94941

Closed

serathius mentioned this pull request

etcd client v3 v3.5.0 sends invalid :authority header field value #13192

Closed

lavacat mentioned this pull request

[3.4] clientv3/balancer: fixed flaky TestRoundRobinBalancedResolvableFailoverFromServerFail #14274

Merged

ahrtr mentioned this pull request

Remove the dependency on grpc-go's experimental API (e.g. resolver & balancer) #15145

Open

CaojiamingAlan mentioned this pull request

Improving client call options flexibility and replace retry interceptor #16216

Open

chaochn47 mentioned this pull request

dependency: bump google.golang.org/grpc from 1.51.0 to 1.52.0 #15131

Closed

ahrtr mentioned this pull request

Resolve CVE-2023-44487 #16740

Closed

24 tasks

chaochn47 mentioned this pull request

[3.4] Backport clientv3: Replace balancer with upstream grpc solution #16827

Closed

chaochn47 added a commit to chaochn47/etcd that referenced this pull request


          [3.4] Backport etcd-io#12671 clientv3: Replace balancer with upstream…

b239cf6

… grpc solution

Signed-off-by: Chao Chen <chaochn@amazon.com>

chaochn47 added a commit to chaochn47/etcd that referenced this pull request


          [3.4] Backport etcd-io#12671 clientv3: Replace balancer with upstream…

d44b29e

… grpc solution

Signed-off-by: Chao Chen <chaochn@amazon.com>

chaochn47 added a commit to chaochn47/etcd that referenced this pull request


          [3.4] Backport etcd-io#12671 clientv3: Replace balancer with upstream…

6e6753c

… grpc solution

Signed-off-by: Chao Chen <chaochn@amazon.com>

chaochn47 mentioned this pull request

[3.4] Backport #12671 clientv3: Replace balancer with upstream grpc solution #16844

Merged

chaochn47 added a commit to chaochn47/etcd that referenced this pull request


          [3.4] Backport etcd-io#12671 clientv3: Replace balancer with upstream…

83da5ff

… grpc solution

Signed-off-by: Chao Chen <chaochn@amazon.com>

ahrtr added a commit that referenced this pull request


          Merge pull request #16844 from chaochn47/release-3.4-replace-balancer

0fb0045

[3.4] Backport #12671 clientv3: Replace balancer with upstream grpc solution

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