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

tests: Add new test for round robin resolver #15577

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

jmhbnz
Copy link
Member

@jmhbnz jmhbnz commented Mar 27, 2023

The default grpc-go load balancing strategy is pick first which may not be expected, particularly for production scenarios. We should consider adding a test for round robin as an alternative.

Refer: etcd-io/website#633

@jmhbnz
Copy link
Member Author

jmhbnz commented Mar 27, 2023

Useful context on grpc-go resolver within etcd: #15145

This pr may not be required based on the above issue, or alternatively be reverted at a later date once a new approach is merged.

Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

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

Thank you. Looks good. But I think that the number-of-calls assertion is crucial to verify it's really RR.

Probably most of the code could be shared with the previous test... just make the 'policy' options a parameter and post-run verification.

@jmhbnz jmhbnz force-pushed the add-round-robin-test branch 5 times, most recently from 7eac0e5 to a90d2d0 Compare March 29, 2023 10:39
@jmhbnz jmhbnz changed the title Add new test for round robin resolver tests: Add new test for round robin resolver Mar 29, 2023
@jmhbnz jmhbnz marked this pull request as ready for review March 29, 2023 19:38
@jmhbnz jmhbnz force-pushed the add-round-robin-test branch 2 times, most recently from ce7a57a to de265d9 Compare March 29, 2023 19:58
@tjungblu
Copy link
Contributor

(non-binding) /lgtm

@jmhbnz jmhbnz force-pushed the add-round-robin-test branch 2 times, most recently from e69ff39 to 7261184 Compare April 19, 2023 20:39
@jmhbnz
Copy link
Member Author

jmhbnz commented Apr 24, 2023

Hey @serathius, @ahrtr - This pull request updates our existing test case testEtcdGrpcResolver to verify the resolver works as expected when using a round_robin strategy in addition to the default pick_first strategy.

I'm conscious we have related discussion in #15145 about potentially moving away from the grpc-go experimental resolver so can you please take a look and let me know if you think this pull request is worth merging? If not we can just close no problem.

In my mind reading the issue linked above it sounds like we might still be with grpc-go resolver for a while so I think a bit extra test coverage isn't a bad thing, but I will defer to your judgement here, thanks! 🙏🏻

@ahrtr
Copy link
Member

ahrtr commented Apr 25, 2023

Please also rebase this PR.

@jmhbnz jmhbnz force-pushed the add-round-robin-test branch from 7261184 to 18e3aca Compare April 25, 2023 06:44
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

Thanks @jmhbnz

@ahrtr ahrtr merged commit 4485db3 into etcd-io:main Apr 25, 2023
@jmhbnz jmhbnz deleted the add-round-robin-test branch July 27, 2023 02:13
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.

4 participants