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

add(cmd/ghz): expose --lb-strategy #259

Merged
merged 1 commit into from
Feb 12, 2021
Merged

Conversation

CAFxX
Copy link
Contributor

@CAFxX CAFxX commented Feb 5, 2021

Fixes #258 (at least the MVP)

@CAFxX CAFxX changed the title add(cmd/ghz): expose --lb-strategy add(cmd/ghz): expose --lb-strategy Feb 5, 2021
@bojand
Copy link
Owner

bojand commented Feb 8, 2021

Hello, thanks for the PR. Unfortunately load balancing in gRPC is a complicated topic. The reason #242 was API-only change is because even when using the included round robin LB strategy, additional setup might be (usually is) needed by the user. My understanding is that specifically a resolver would need to be registered via init side effect (See example). By default grpc-go uses a a passthrough resolver, so even setting the balancing strategy like this, depending on the configuration and hostname used, it would likely end up resulting in picking the first instance. The workaround is to use dns:///... scheme in passed hostname, or alternatively set the default resolver to "dns" (see docs).

So i think there is a little bit of additional work needed in this PR. in main() after we set up the config, where we create the logger, before Run() we should check if LB flag was set, and if it is round-robin we should set the resolver to dns using resolver.SetDefaultScheme("dns").

@CAFxX
Copy link
Contributor Author

CAFxX commented Feb 8, 2021

We can simply document that if the user wants to use client load balancing, she should specify the hostname with the dns:/// prefix. That's what we do internally.

Optionally we can add a warning if the hostname is missing the prefix.

@bojand bojand merged commit ce92238 into bojand:master Feb 12, 2021
@bojand
Copy link
Owner

bojand commented Feb 12, 2021

Hello, thanks for the PR. This has been published in 0.91.0.

@mjgp2
Copy link

mjgp2 commented Jun 14, 2022

I don't think the dns:/// is documented fwiw

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.

Expose client load balancing policy via command line
3 participants