-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
dns: stop polling for updates; use UpdateState API #3165
Conversation
This PR is ready for review now that #3186 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.
Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @dfawley and @menghanl)
internal/resolver/dns/dns_resolver.go, line 297 at r1 (raw file):
Addresses: append(d.lookupHost(), srv...), } // Support fallback to non-balancer address.
Move this comment above append
.
And make it clear what it means? Addresses contain fallback non-balancer addresses from host and balancer addresses from SRV
Or just delete it.
internal/resolver/dns/dns_resolver_test.go, line 753 at r1 (raw file):
} sc := scFromState(state) if !reflect.DeepEqual(a.scWant, sc) {
This is comparing strings, ==
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.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @menghanl)
internal/resolver/dns/dns_resolver.go, line 297 at r1 (raw file):
Addresses contain fallback non-balancer addresses from host and balancer addresses from SRV
Deleted, since we will be getting rid of the balancer addresses in the address list soon.
internal/resolver/dns/dns_resolver_test.go, line 753 at r1 (raw file):
Previously, menghanl (Menghan Li) wrote…
This is comparing strings,
==
Done.
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.
Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved
This cannot be merged until the pickfirst and round_robin balancers are updated to return an error from
UpdateState
when zero addresses are provided, as we currently rely upon the polling behavior of DNS to refresh on error.This change is