-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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 cooldown mechanism #14228
DNS cooldown mechanism #14228
Conversation
|
@@ -16,23 +16,46 @@ | |||
* |
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.
After talking to Yuchen, I've learnt that resolve_address_test.cc
is a much better place for testing. I'll port these changes there tomorrow. The idea will the the same though: a chain of callbacks that will verify the behavior.
|
|
The native DNS resolver code looks pretty decent. Most of my comments are cosmetic; the only really substantive one is the issue about when to use the backoff vs. when to use the minimum time between requests. Please let me know if you have any questions. Thanks! Reviewed 2 of 3 files at r1. include/grpc/impl/codegen/grpc_types.h, line 243 at r1 (raw file):
Terminology question: Is "period" the right term here? To me, that implies that there's an event that repeatedly occurs at some fixed interval, which isn't the case here. Perhaps a better name for this would be something like src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 77 at r1 (raw file):
Suggest calling this something like src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 77 at r1 (raw file):
It looks like this code is using src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 82 at r1 (raw file):
I think we can reuse the existing retry timer for this. I don't think the two timers will both be active at the same time, and they both serve essentially the same purpose (i.e., time until the next resolution attempt). I suggest calling the combined timer something like src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 85 at r1 (raw file):
Note that even though we can reuse the retry timer for this, we probably will need to keep the closures separate, because we want slightly different behavior when the timer fires in the two cases (for details, see my comment below in I suggest calling this something like src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 123 at r1 (raw file):
Not directly related to this PR, but I think this is the wrong place to do this. I think the only place we should reset the backoff state is in src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 136 at r1 (raw file):
Same here. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 204 at r1 (raw file):
I'm not sure that we want to do this check in every case where we call this function. We do want to do this when we're called from To say this another way, I think we want to use the minimum time between requests when the last request was successful, but when the last request failed, we want to use the backoff code. At any given time, we should be using only one of the two mechanisms, depending on whether the last request was a success or a failure. To do this, I think we need to move this new logic out into its own function called The code in src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 209 at r1 (raw file):
Instead of calling src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 224 at r1 (raw file):
I don't think it makes sense to do this here, since we don't actually have any new data yet. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 238 at r1 (raw file):
Please use test/core/client_channel/resolvers/dns_resolver_test.cc, line 16 at r1 (raw file): Previously, dgquintas (David G. Quintas) wrote…
Okay, I'll hold off on reviewing the test until you make those changes. Comments from Reviewable |
|
|
bd91d0c
to
956fe5e
Compare
|
Review status: 2 of 3 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed. include/grpc/impl/codegen/grpc_types.h, line 243 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 77 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 77 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 82 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 85 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 123 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
I'd rather not make changes unrelated to the purpose of this PR. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 136 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
I'd rather not make changes unrelated to the purpose of this PR. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 204 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 209 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 224 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
This is meant to trigger the notification using the already present data: even though we don't re-resolve, we want to get back to the client of this code with the last "cached" copy of the resolution. The alternative is to delay the code using the resolver, which seems undesirable. Note that we schedule a future resolution at the earliest possible time anyway. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 238 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. test/core/client_channel/resolvers/dns_resolver_test.cc, line 16 at r1 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Added tests and c-ares changes. Comments from Reviewable |
|
|
1 similar comment
|
|
1 similar comment
|
|
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 99 at r6 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 384 at r6 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 83 at r5 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. Comments from Reviewable |
|
|
Reviewed 2 of 2 files at r7. src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 384 at r6 (raw file): Previously, dgquintas (David G. Quintas) wrote…
Doesn't look like this was done -- the function is still here. src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 432 at r7 (raw file):
This does not need to be initialized again. It's already being set on line 421 above. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 179 at r7 (raw file):
This doesn't need to be initialized here, because that's already been done on line 292 below. Comments from Reviewable |
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 384 at r6 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Bad commit. PTAL. src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 432 at r7 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 179 at r7 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. But in any case, you seem to be looking at the wrong commit (I did rename "on_retry"). Comments from Reviewable |
|
|
|
|
Sorry, found a few more minor problems. Reviewed 2 of 2 files at r8. src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 96 at r8 (raw file):
It's initialized to -1, not src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 150 at r8 (raw file):
This should call src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 308 at r8 (raw file):
This should call src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 381 at r8 (raw file):
Nit: Unnecessary blank line. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 79 at r8 (raw file):
It's initialized to -1, not Comments from Reviewable |
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 96 at r8 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 150 at r8 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 308 at r8 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 381 at r8 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 79 at r8 (raw file): Previously, markdroth (Mark D. Roth) wrote…
Done. Comments from Reviewable |
|
|
|
Looks great! Reviewed 2 of 2 files at r9. Comments from Reviewable |
|
|
|
|
|
|
Green! 🍾 |
We want to allow code to hit the resolver as often as it wants. To avoid overloading the system-level resolution mechanisms as well as the remote nameservers, this PR introduces the concept of minimum time between resolutions: any resolution request happening before this cooldown period is over will get a copy of the previous results if any are available, and schedule a resolution at the earliest possible time.
This change is