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

grpc_naming.md round-robin strategy #633

Merged
merged 1 commit into from
Mar 28, 2023
Merged

grpc_naming.md round-robin strategy #633

merged 1 commit into from
Mar 28, 2023

Conversation

ramil600
Copy link
Contributor

@ramil600 ramil600 commented Dec 2, 2022

when using new resolver from /client/v3/naming/resolver , default grpc load balancing strategy: pick first will be used. It would be nice to point out how to use round-robin load balancing.

Signed-off-by: Ramil Mirhasanov [email protected]

…ick first will be used. It would be nice to point out how to use round-robin load balancing.

Signed-off-by: Ramil Mirhasanov <[email protected]>
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.

Thanks for raising @ramil600, apologies for the delay responding to this, etcd has not had enough maintainers to respond to all issues.

This change looks reasonable to me. I agree the default of pick first may not be expected.

@ahrtr , @spzala - Can you please take a look at this one?

@ptabor
Copy link
Contributor

ptabor commented Mar 27, 2023

LGTM. But ideally we should have a test if we are making it an officially supported use-case.
I don't recall test using the Round Robin policy.

@jmhbnz
Copy link
Member

jmhbnz commented Mar 27, 2023

LGTM. But ideally we should have a test if we are making it an officially supported use-case. I don't recall test using the Round Robin policy.

Good point! I've opened a draft here for a new case etcd-io/etcd#15577. We can finish and merge that if there is broader consensus that it's worthwhile to support round robin.

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.

LGTM with a comment, thank you @jmhbnz

cc @spzala


```go

conn, gerr := grpc.Dial("etcd:///foo", grpc.WithResolvers(etcdResolver),
Copy link
Member

Choose a reason for hiding this comment

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

Actually Resolver is an experimental gRPC API, and we need to remove the dependency on it. FYI. etcd-io/etcd#15145

I am OK to merge this PR firstly, and update it when etcd-io/etcd#15145 is done.

cc @spzala

Copy link
Member

Choose a reason for hiding this comment

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

@ahrtr thanks and that sounds good to me too!

Copy link
Member

@spzala spzala left a comment

Choose a reason for hiding this comment

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


```go

conn, gerr := grpc.Dial("etcd:///foo", grpc.WithResolvers(etcdResolver),
Copy link
Member

Choose a reason for hiding this comment

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

@ahrtr thanks and that sounds good to me too!

@spzala spzala merged commit 7ff416a into etcd-io:main Mar 28, 2023
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.

5 participants