-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Conversation
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. |
There was a problem hiding this 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.
7eac0e5
to
a90d2d0
Compare
ce7a57a
to
de265d9
Compare
(non-binding) /lgtm |
e69ff39
to
7261184
Compare
Hey @serathius, @ahrtr - This pull request updates our existing test case 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! 🙏🏻 |
Please also rebase this PR. |
Signed-off-by: James Blair <mail@jamesblair.net>
7261184
to
18e3aca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @jmhbnz
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