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

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

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

chaochn47
Copy link
Member

@chaochn47 chaochn47 commented Oct 27, 2023

Breakdown of #16827

Backports

Changes made on top of the original PR:

  • Unrelated go.mod and go.sum files are removed
  • Internal resolver and endpoints packages are moved into clientv3 path
  • Stripped grpc proxy changes

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@chaochn47
Copy link
Member Author

chaochn47 commented Oct 27, 2023

Proxy test failed. https://github.com/etcd-io/etcd/actions/runs/6672706002/job/18137135993?pr=16844

I have to backport its follow up PR to make CI green.

@chaochn47 chaochn47 marked this pull request as ready for review October 27, 2023 23:17
@ahrtr ahrtr mentioned this pull request Oct 28, 2023
24 tasks
clientv3/client.go Outdated Show resolved Hide resolved
@ahrtr
Copy link
Member

ahrtr commented Oct 28, 2023

Stripped grpc proxy changes

There are two changes with grpc proxy in #12706,

  • proxy/grpcproxy/health.go
    It should be fine to ignore this change, because the c will never be nil.
  • etcdmain/grpc_proxy.go
    Why stripped this change?

@chaochn47
Copy link
Member Author

chaochn47 commented Oct 29, 2023

Stripped grpc proxy changes

There are two changes with grpc proxy in #12706,

* proxy/grpcproxy/health.go
  It should be fine to ignore this change, because the `c` will never be nil.

* etcdmain/grpc_proxy.go
  Why stripped this change?

etcdmain/grpc_proxy.go change is unrelated to unblock the CVE change. The client was created for proxy health check in #12114.

I assume we want to reduce the moving parts for the sake of bumping up gRPC versions, so I stripped this change.

Does that make sense to you? @ahrtr

@ahrtr
Copy link
Member

ahrtr commented Oct 29, 2023

The client was created for proxy health check in #12114.

OK, This PR (a feature) hasn't been backported to 3.4 at all, so no need to backport the grpc-proxy related change in #12706 to 3.4 either.

@chaochn47
Copy link
Member Author

chaochn47 commented Oct 29, 2023

The client was created for proxy health check in #12114.

OK, This PR (a feature) hasn't been backported to 3.4 at all, so no need to backport the grpc-proxy related change in #12706 to 3.4 either.

Yeah, I could have documented it clearer.

@ahrtr Other than this contentious change, is there anything else I should follow up for this PR?

@ahrtr
Copy link
Member

ahrtr commented Oct 29, 2023

@ahrtr Other than this contentious change, is there anything else I should follow up for this PR?

Overall looks good to me. The only minor comment is #16844 (comment)

Thanks.

@ahrtr
Copy link
Member

ahrtr commented Oct 30, 2023

Just merged #16849, please rebase this PR.

… grpc solution

Signed-off-by: Chao Chen <chaochn@amazon.com>
…ream grpc solution

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

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

Backports LGTM - Thanks @chaochn47

@ahrtr ahrtr merged commit 0fb0045 into etcd-io:release-3.4 Oct 31, 2023
12 checks passed
@chaochn47 chaochn47 deleted the release-3.4-replace-balancer branch October 31, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants