DNS cooldown mechanism #14228

dgquintas · 2018-01-30T01:40:44Z

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

grpc-testing · 2018-01-30T01:43:58Z

****************************************************************

libgrpc.so

     VM SIZE                                                                               FILE SIZE
 ++++++++++++++ GROWING                                                                 ++++++++++++++
  +0.0%    +240 [None]                                                                  +2.32Ki  +0.0%
   +27%    +560 src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc    +560   +27%
      +182%    +315 dns_start_resolving_locked                                                 +315  +182%
       +39%    +144 dns_factory_create_resolver                                                +144   +39%
       +25%     +48 dns_shutdown_locked                                                         +48   +25%
      [NEW]     +45 cooldown_cb                                                                 +45  [NEW]
      +8.1%      +8 [Unmapped]                                                                   +8  +8.1%

  +0.1%    +800 TOTAL                                                                   +2.87Ki  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

dgquintas · 2018-01-30T01:44:33Z

test/core/client_channel/resolvers/dns_resolver_test.cc

@@ -16,23 +16,46 @@
 *


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.

grpc-testing · 2018-01-30T01:55:59Z

[trickle] No significant performance differences

grpc-testing · 2018-01-30T02:35:30Z

[microbenchmarks] No significant performance differences

markdroth · 2018-01-30T16:06:04Z

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.
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):

  "grpc.initial_reconnect_backoff_ms"
/** Minimum amount of time between DNS resolutions, in ms */
#define GRPC_ARG_DNS_MIN_RESOLUTION_PERIOD_MS \

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 DNS_MIN_TIME_BETWEEN_RESOLUTIONS_MS?

src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 77 at r1 (raw file):

  grpc_core::ManualConstructor<grpc_core::BackOff> backoff;
  /** min resolution period. Max one resolution will happen per period */
  gpr_timespec cooldown_period;

Suggest calling this something like min_time_between_resolutions.

src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 77 at r1 (raw file):

  grpc_core::ManualConstructor<grpc_core::BackOff> backoff;
  /** min resolution period. Max one resolution will happen per period */
  gpr_timespec cooldown_period;

It looks like this code is using gpr_timespec all over the place. I think we should be using grpc_millis instead.

src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 82 at r1 (raw file):

  gpr_timespec last_resolution_timestamp;
  /** Timer for resolutions delayed due to cooldown period */
  grpc_timer cooldown_timer;

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 next_resolution_timer.

src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 85 at r1 (raw file):

  bool have_cooldown_timer;
  /** To be invoked once the cooldown period is over */
  grpc_closure cooldown_closure;

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 dns_start_resolving_locked()).

I suggest calling this something like deferred_resolution_closure.

src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 123 at r1 (raw file):

  dns_resolver* r = (dns_resolver*)resolver;
  if (!r->resolving) {
    r->backoff->Reset();

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 dns_on_resolved_locked(), if the DNS request was successful. Otherwise, if we are already retrying when channel_saw_error_locked() is called, we will reset the backoff state and use the incorrect backoff for the next attempt.

src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 136 at r1 (raw file):

  r->target_result = target_result;
  if (r->resolved_version == 0 && !r->resolving) {
    r->backoff->Reset();

Same here.

src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 204 at r1 (raw file):

static void dns_start_resolving_locked(dns_resolver* r) {
  if (gpr_time_cmp(gpr_inf_past(GPR_CLOCK_MONOTONIC),

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 channel_saw_error_locked() and next_locked(0. And while it's not harmful to do this when we're called from cooldown_cb(), it's also not necessary, because we already know that it's been long enough since the last request that we can now issue a new one. But the real problem is that when we're called from dns_on_retry_timer_locked(): in that case, we do not want to do this check, because that would interfere with the backoff logic.

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 maybe_start_resolving_locked(). That new function will do this new check to see whether we need to set a timer. If we do, it will set the timer and return; otherwise, it will call this function.

The code in channel_saw_error_locked() and next_locked() can call the new maybe_start_resolving_locked() instead of calling this one. The code in cooldown_cb() and dns_on_retry_timer_locked() can call this function directly.

src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 209 at r1 (raw file):

        gpr_time_add(r->last_resolution_timestamp, r->cooldown_period);
    const auto ms_until_next_resolution = gpr_time_to_millis(
        gpr_time_sub(earliest_next_resolution, gpr_now(GPR_CLOCK_MONOTONIC)));

Instead of calling gpr_now(), let's use grpc_core::ExecCtx::Get()->Now().

src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 224 at r1 (raw file):

  ++r->resolved_version;
  dns_maybe_finish_next_locked(r);

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):

                          grpc_combiner_scheduler(r->base.combiner)),
      &r->addresses);
  r->last_resolution_timestamp = gpr_now(GPR_CLOCK_MONOTONIC);

Please use grpc_core::ExecCtx::Get()->Now().

test/core/client_channel/resolvers/dns_resolver_test.cc, line 16 at r1 (raw file):

Previously, dgquintas (David G. Quintas) wrote…

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.

Okay, I'll hold off on reviewing the test until you make those changes.

Comments from Reviewable

grpc-testing · 2018-01-30T23:50:43Z

****************************************************************

libgrpc.so

     VM SIZE                                                                                    FILE SIZE
 ++++++++++++++ GROWING                                                                      ++++++++++++++
  +0.1%    +416 [None]                                                                       +4.19Ki  +0.1%
      +0.1%    +378 [Unmapped]                                                                   +4.15Ki  +0.1%
      [NEW]     +38 dns_ares_maybe_start_resolving_locked(ares_dns_resolver*)::__func__              +38  [NEW]
   +13%    +448 src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc    +448   +13%
      [NEW]    +244 dns_ares_maybe_start_resolving_locked                                           +244  [NEW]
       +24%    +112 dns_factory_create_resolver                                                     +112   +24%
      [NEW]     +48 ares_cooldown_cb                                                                 +48  [NEW]
      [NEW]     +48 dns_ares_on_next_resolution_timer_locked                                         +48  [NEW]
      +1.9%     +32 dns_ares_on_resolved_locked                                                      +32  +1.9%
      +9.8%     +12 [Unmapped]                                                                       +12  +9.8%
   +18%    +384 src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc         +384   +18%
      [NEW]    +188 maybe_start_resolving_locked                                                    +188  [NEW]
       +33%    +120 dns_factory_create_resolver                                                     +120   +33%
      [NEW]     +45 cooldown_cb                                                                      +45  [NEW]
      [NEW]     +45 dns_on_next_resolution_timer_locked                                              +45  [NEW]
       +19%     +33 dns_start_resolving_locked                                                       +33   +19%

  +0.1% +1.22Ki TOTAL                                                                        +5.00Ki  +0.1%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-01-31T00:02:46Z

[trickle] No significant performance differences

grpc-testing · 2018-01-31T00:09:38Z

****************************************************************

libgrpc.so

     VM SIZE                                                                                    FILE SIZE
 ++++++++++++++ GROWING                                                                      ++++++++++++++
  +0.1%    +416 [None]                                                                       +4.19Ki  +0.1%
      +0.1%    +378 [Unmapped]                                                                   +4.15Ki  +0.1%
      [NEW]     +38 dns_ares_maybe_start_resolving_locked(ares_dns_resolver*)::__func__              +38  [NEW]
   +13%    +448 src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc    +448   +13%
      [NEW]    +244 dns_ares_maybe_start_resolving_locked                                           +244  [NEW]
       +24%    +112 dns_factory_create_resolver                                                     +112   +24%
      [NEW]     +48 ares_cooldown_cb                                                                 +48  [NEW]
      [NEW]     +48 dns_ares_on_next_resolution_timer_locked                                         +48  [NEW]
      +1.9%     +32 dns_ares_on_resolved_locked                                                      +32  +1.9%
      +9.8%     +12 [Unmapped]                                                                       +12  +9.8%
   +18%    +384 src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc         +384   +18%
      [NEW]    +188 maybe_start_resolving_locked                                                    +188  [NEW]
       +33%    +120 dns_factory_create_resolver                                                     +120   +33%
      [NEW]     +45 cooldown_cb                                                                      +45  [NEW]
      [NEW]     +45 dns_on_next_resolution_timer_locked                                              +45  [NEW]
       +19%     +33 dns_start_resolving_locked                                                       +33   +19%

  +0.1% +1.22Ki TOTAL                                                                        +5.00Ki  +0.1%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

dgquintas · 2018-01-31T00:09:44Z

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…

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 DNS_MIN_TIME_BETWEEN_RESOLUTIONS_MS?

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…

Suggest calling this something like min_time_between_resolutions.

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…

It looks like this code is using gpr_timespec all over the place. I think we should be using grpc_millis instead.

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…

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 next_resolution_timer.

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…

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 dns_start_resolving_locked()).

I suggest calling this something like deferred_resolution_closure.

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…

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 dns_on_resolved_locked(), if the DNS request was successful. Otherwise, if we are already retrying when channel_saw_error_locked() is called, we will reset the backoff state and use the incorrect backoff for the next attempt.

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…

Same here.

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…

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 channel_saw_error_locked() and next_locked(0. And while it's not harmful to do this when we're called from cooldown_cb(), it's also not necessary, because we already know that it's been long enough since the last request that we can now issue a new one. But the real problem is that when we're called from dns_on_retry_timer_locked(): in that case, we do not want to do this check, because that would interfere with the backoff logic.

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 maybe_start_resolving_locked(). That new function will do this new check to see whether we need to set a timer. If we do, it will set the timer and return; otherwise, it will call this function.

The code in channel_saw_error_locked() and next_locked() can call the new maybe_start_resolving_locked() instead of calling this one. The code in cooldown_cb() and dns_on_retry_timer_locked() can call this function directly.

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…

Instead of calling gpr_now(), let's use grpc_core::ExecCtx::Get()->Now().

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…

  ++r->resolved_version;
  dns_maybe_finish_next_locked(r);
I don't think it makes sense to do this here, since we don't actually have any new data yet.

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…

Please use grpc_core::ExecCtx::Get()->Now().

Done.

test/core/client_channel/resolvers/dns_resolver_test.cc, line 16 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Okay, I'll hold off on reviewing the test until you make those changes.

Added tests and c-ares changes.

Comments from Reviewable

grpc-testing · 2018-01-31T00:16:12Z

****************************************************************

libgrpc.so

     VM SIZE                                                                                    FILE SIZE
 ++++++++++++++ GROWING                                                                      ++++++++++++++
  +0.1%    +368 [None]                                                                       +3.98Ki  +0.1%
   +12%    +400 src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc    +400   +12%
      [NEW]    +204 dns_ares_maybe_start_resolving_locked                                           +204  [NEW]
       +24%    +112 dns_factory_create_resolver                                                     +112   +24%
      [NEW]     +48 ares_cooldown_cb                                                                 +48  [NEW]
      [NEW]     +48 dns_ares_on_next_resolution_timer_locked                                         +48  [NEW]
      +1.9%     +32 dns_ares_on_resolved_locked                                                      +32  +1.9%
      +3.3%      +4 [Unmapped]                                                                        +4  +3.3%
   +18%    +384 src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc         +384   +18%
      [NEW]    +188 maybe_start_resolving_locked                                                    +188  [NEW]
       +33%    +120 dns_factory_create_resolver                                                     +120   +33%
      [NEW]     +45 cooldown_cb                                                                      +45  [NEW]
      [NEW]     +45 dns_on_next_resolution_timer_locked                                              +45  [NEW]
       +19%     +33 dns_start_resolving_locked                                                       +33   +19%

  +0.1% +1.12Ki TOTAL                                                                        +4.75Ki  +0.1%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-01-31T00:21:29Z

[trickle] No significant performance differences

grpc-testing · 2018-01-31T00:27:08Z

[trickle] No significant performance differences

grpc-testing · 2018-01-31T00:42:11Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-01-31T01:01:36Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-01-31T01:08:04Z

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_unary_ping_pong.BM_UnaryPingPong_InProcessCHTTP2_NoOpMutator_NoOpMutator__512_512.opt.old: 1


[microbenchmarks] No significant performance differences

dgquintas · 2018-02-01T16:59:24Z

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…

Doesn't look like this field was actually removed. It's not needed; we can use dns_ares_on_next_resolution_timer_locked for both purposes.

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…

This is no longer needed. It's functionally identical to dns_ares_on_next_resolution_timer_locked().

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…

The closure is still here in the struct. It's not needed; we can use the on_retry closure instead (which should probably be renamed to next_resolution_closure).

Done.

Comments from Reviewable

grpc-testing · 2018-02-01T17:02:04Z

****************************************************************

libgrpc.so

     VM SIZE                                                                                    FILE SIZE
 ++++++++++++++ GROWING                                                                      ++++++++++++++
  +0.1%    +448 [None]                                                                       +5.00Ki  +0.1%
   +13%    +448 src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc    +448   +13%
      [NEW]    +210 dns_ares_maybe_start_resolving_locked                                           +210  [NEW]
       +24%    +112 dns_factory_create_resolver                                                     +112   +24%
      [NEW]     +48 ares_cooldown_cb                                                                 +48  [NEW]
      [NEW]     +48 dns_ares_on_next_resolution_timer_locked                                         +48  [NEW]
      +1.9%     +32 dns_ares_on_resolved_locked                                                      +32  +1.9%
       +95%     +20 dns_ares_channel_saw_error_locked                                                +20   +95%
       +10%     +15 dns_ares_next_locked                                                             +15   +10%
      +8.7%     +11 [Unmapped]                                                                       +11  +8.7%
   +21%    +432 src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc         +432   +21%
      [NEW]    +202 maybe_start_resolving_locked                                                    +202  [NEW]
       +33%    +120 dns_factory_create_resolver                                                     +120   +33%
      [NEW]     +45 cooldown_cb                                                                      +45  [NEW]
      [NEW]     +45 dns_on_next_resolution_timer_locked                                              +45  [NEW]
       +19%     +33 dns_start_resolving_locked                                                       +33   +19%
       +22%     +20 dns_next_locked                                                                  +20   +22%
       +95%     +20 dns_channel_saw_error_locked                                                     +20   +95%

  +0.1% +1.30Ki TOTAL                                                                        +5.86Ki  +0.1%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-02-01T17:13:44Z

[trickle] No significant performance differences

markdroth · 2018-02-01T17:19:26Z

Reviewed 2 of 2 files at r7.
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, dgquintas (David G. Quintas) wrote…

Done.

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):

      grpc_channel_arg_get_integer(period_arg, {1000, 0, INT_MAX});
  r->last_resolution_timestamp = -1;
  GRPC_CLOSURE_INIT(&r->dns_ares_on_next_resolution_timer_closure,

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):

      gpr_log(GPR_DEBUG, "retrying immediately");
    }
    GRPC_CLOSURE_INIT(&r->next_resolution_closure,

This doesn't need to be initialized here, because that's already been done on line 292 below.

Comments from Reviewable

dgquintas · 2018-02-01T17:27:16Z

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…

Doesn't look like this was done -- the function is still here.

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…

This does not need to be initialized again. It's already being set on line 421 above.

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…

This doesn't need to be initialized here, because that's already been done on line 292 below.

Done. But in any case, you seem to be looking at the wrong commit (I did rename "on_retry").

Comments from Reviewable

grpc-testing · 2018-02-01T17:29:49Z

****************************************************************

libgrpc.so

     VM SIZE                                                                                    FILE SIZE
 ++++++++++++++ GROWING                                                                      ++++++++++++++
  +0.1%    +448 [None]                                                                       +5.00Ki  +0.1%
   +13%    +448 src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc    +448   +13%
      [NEW]    +210 dns_ares_maybe_start_resolving_locked                                           +210  [NEW]
       +24%    +112 dns_factory_create_resolver                                                     +112   +24%
      [NEW]     +48 ares_cooldown_cb                                                                 +48  [NEW]
      [NEW]     +48 dns_ares_on_next_resolution_timer_locked                                         +48  [NEW]
      +1.9%     +32 dns_ares_on_resolved_locked                                                      +32  +1.9%
       +95%     +20 dns_ares_channel_saw_error_locked                                                +20   +95%
       +10%     +15 dns_ares_next_locked                                                             +15   +10%
      +8.7%     +11 [Unmapped]                                                                       +11  +8.7%
   +21%    +432 src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc         +432   +21%
      [NEW]    +202 maybe_start_resolving_locked                                                    +202  [NEW]
       +33%    +120 dns_factory_create_resolver                                                     +120   +33%
      [NEW]     +45 cooldown_cb                                                                      +45  [NEW]
      [NEW]     +45 dns_on_next_resolution_timer_locked                                              +45  [NEW]
       +19%     +33 dns_start_resolving_locked                                                       +33   +19%
       +22%     +20 dns_next_locked                                                                  +20   +22%
       +95%     +20 dns_channel_saw_error_locked                                                     +20   +95%

  +0.1% +1.30Ki TOTAL                                                                        +5.86Ki  +0.1%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-02-01T17:41:12Z

[trickle] No significant performance differences

grpc-testing · 2018-02-01T17:54:34Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-02-01T18:21:23Z

[microbenchmarks] Performance differences noted:
Benchmark                                                                                 cpu_time    real_time
----------------------------------------------------------------------------------------  ----------  -----------
BM_StreamingPingPong<MinInProcess, NoOpMutator, NoOpMutator>/262144/1                     +5%         +5%
BM_StreamingPingPong<MinInProcess, NoOpMutator, NoOpMutator>/262144/2                     +5%         +5%
BM_StreamingPingPongWithCoalescingApi<InProcess, NoOpMutator, NoOpMutator>/262144/1/0     +5%         +5%
BM_StreamingPingPongWithCoalescingApi<MinInProcess, NoOpMutator, NoOpMutator>/262144/2/0  +4%         +4%
BM_StreamingPingPongWithCoalescingApi<MinInProcess, NoOpMutator, NoOpMutator>/262144/2/1  +7%         +7%

markdroth · 2018-02-01T19:24:03Z

Sorry, found a few more minor problems.

Reviewed 2 of 2 files at r8.
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):

  grpc_millis min_time_between_resolutions;
  /** when was the last resolution? If no resolution has happened yet, equals
   * gpr_inf_past() */

It's initialized to -1, not gpr_inf_past().

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 150 at r8 (raw file):

  if (error == GRPC_ERROR_NONE) {
    if (!r->resolving) {
      dns_ares_maybe_start_resolving_locked(r);

This should call dns_ares_start_resolving_locked(), not dns_ares_maybe_start_resolving_locked().

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 308 at r8 (raw file):

  if (r->resolved_version == 0 && !r->resolving) {
    r->backoff->Reset();
    dns_ares_start_resolving_locked(r);

This should call dns_ares_maybe_start_resolving_locked() instead of dns_ares_start_resolving_locked().

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc, line 381 at r8 (raw file):

  gpr_free(r);
}

Nit: Unnecessary blank line.

src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc, line 79 at r8 (raw file):

  grpc_millis min_time_between_resolutions;
  /** when was the last resolution? If no resolution has happened yet, equals
   * gpr_inf_past() */

It's initialized to -1, not gpr_inf_past().

Comments from Reviewable

dgquintas · 2018-02-01T21:45:10Z

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…

It's initialized to -1, not gpr_inf_past().

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…

This should call dns_ares_start_resolving_locked(), not dns_ares_maybe_start_resolving_locked().

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…

This should call dns_ares_maybe_start_resolving_locked() instead of dns_ares_start_resolving_locked().

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…

Nit: Unnecessary blank line.

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…

It's initialized to -1, not gpr_inf_past().

Done.

Comments from Reviewable

grpc-testing · 2018-02-01T21:48:19Z

****************************************************************

libgrpc.so

     VM SIZE                                                                                    FILE SIZE
 ++++++++++++++ GROWING                                                                      ++++++++++++++
  +0.1%    +384 [None]                                                                       +4.91Ki  +0.1%
      +0.1%    +384 [Unmapped]                                                                   +4.91Ki  +0.1%
      +0.1%      +1 [None]                                                                             0  [ = ]
   +13%    +448 src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc    +448   +13%
      [NEW]    +210 dns_ares_maybe_start_resolving_locked                                           +210  [NEW]
       +24%    +112 dns_factory_create_resolver                                                     +112   +24%
      [NEW]     +48 ares_cooldown_cb                                                                 +48  [NEW]
      [NEW]     +48 dns_ares_on_next_resolution_timer_locked                                         +48  [NEW]
      +1.9%     +32 dns_ares_on_resolved_locked                                                      +32  +1.9%
       +95%     +20 dns_ares_channel_saw_error_locked                                                +20   +95%
       +10%     +15 dns_ares_next_locked                                                             +15   +10%
      +8.7%     +11 [Unmapped]                                                                       +11  +8.7%
   +21%    +432 src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc         +432   +21%
      [NEW]    +202 maybe_start_resolving_locked                                                    +202  [NEW]
       +33%    +120 dns_factory_create_resolver                                                     +120   +33%
      [NEW]     +45 cooldown_cb                                                                      +45  [NEW]
      [NEW]     +45 dns_on_next_resolution_timer_locked                                              +45  [NEW]
       +19%     +33 dns_start_resolving_locked                                                       +33   +19%
       +22%     +20 dns_next_locked                                                                  +20   +22%
       +95%     +20 dns_channel_saw_error_locked                                                     +20   +95%

  +0.1% +1.23Ki TOTAL                                                                        +5.77Ki  +0.1%


****************************************************************

libgrpc++.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++
  [ = ]       0 [None]     +24  +0.0%

  [ = ]       0 TOTAL      +24  +0.0%

grpc-testing · 2018-02-01T22:00:49Z

[trickle] No significant performance differences

grpc-testing · 2018-02-01T22:40:38Z

[microbenchmarks] No significant performance differences

markdroth · 2018-02-01T22:47:47Z

Looks great!

Reviewed 2 of 2 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

grpc-testing · 2018-02-01T23:03:47Z

****************************************************************

libgrpc.so

     VM SIZE                                                                                    FILE SIZE
 ++++++++++++++ GROWING                                                                      ++++++++++++++
  +0.0%    +272 [None]                                                                       +2.31Ki  +0.0%
  +9.3%    +320 src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc    +320  +9.3%
      [NEW]    +196 dns_ares_maybe_start_resolving_locked                                           +196  [NEW]
       +15%     +72 dns_factory_create_resolver                                                      +72   +15%
      [NEW]     +48 dns_ares_on_next_resolution_timer_locked                                         +48  [NEW]
      +1.9%     +32 dns_ares_on_resolved_locked                                                      +32  +1.9%
       +16%     +20 [Unmapped]                                                                       +20   +16%
   +14%    +288 src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc         +288   +14%
      [NEW]    +188 maybe_start_resolving_locked                                                    +188  [NEW]
       +33%    +120 dns_factory_create_resolver                                                     +120   +33%
      [NEW]     +45 dns_on_next_resolution_timer_locked                                              +45  [NEW]
       +19%     +33 dns_start_resolving_locked                                                       +33   +19%

  +0.1%    +880 TOTAL                                                                        +2.91Ki  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-02-01T23:14:51Z

[trickle] No significant performance differences

grpc-testing · 2018-02-01T23:56:03Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-02-02T00:29:44Z

****************************************************************

libgrpc.so

     VM SIZE                                                                                    FILE SIZE
 ++++++++++++++ GROWING                                                                      ++++++++++++++
  +0.0%    +272 [None]                                                                       +2.31Ki  +0.0%
  +9.3%    +320 src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc    +320  +9.3%
      [NEW]    +196 dns_ares_maybe_start_resolving_locked                                           +196  [NEW]
       +15%     +72 dns_factory_create_resolver                                                      +72   +15%
      [NEW]     +48 dns_ares_on_next_resolution_timer_locked                                         +48  [NEW]
      +1.9%     +32 dns_ares_on_resolved_locked                                                      +32  +1.9%
       +16%     +20 [Unmapped]                                                                       +20   +16%
   +14%    +288 src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc         +288   +14%
      [NEW]    +188 maybe_start_resolving_locked                                                    +188  [NEW]
       +33%    +120 dns_factory_create_resolver                                                     +120   +33%
      [NEW]     +45 dns_on_next_resolution_timer_locked                                              +45  [NEW]
       +19%     +33 dns_start_resolving_locked                                                       +33   +19%

  +0.1%    +880 TOTAL                                                                        +2.91Ki  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-02-02T00:40:11Z

[trickle] No significant performance differences

grpc-testing · 2018-02-02T01:21:57Z

[microbenchmarks] No significant performance differences

dgquintas · 2018-02-02T02:43:26Z

Green! 🍾

DNS cooldown mechanism

7896b62

dgquintas added disposition/DO NOT MERGE disposition/Work In Progress labels Jan 30, 2018

dgquintas requested a review from y-zeng January 30, 2018 01:40

dgquintas requested review from a11r and markdroth as code owners January 30, 2018 01:40

dgquintas commented Jan 30, 2018

View reviewed changes

dgquintas mentioned this pull request Jan 30, 2018

Don't destroy null lb_addr #14230

Merged

dgquintas added 3 commits January 30, 2018 15:42

Undo changes to dns_resolver_test.cc

b86cdde

PR Comments

3ee68da

c-ares resolver changes

e7cd835

dgquintas removed disposition/DO NOT MERGE disposition/Work In Progress labels Jan 30, 2018

Added tests, supporting c-ares as well as std resolver.

956fe5e

dgquintas force-pushed the dns_cooldown branch from bd91d0c to 956fe5e Compare January 31, 2018 00:06

Removed debug log

75508a3

Remove spurious closure inits

052c07d

PR comments

e53d6b3

markdroth approved these changes Feb 1, 2018

View reviewed changes

Merge branch 'master' of github.com:grpc/grpc into dns_cooldown

64d02ba

clang-format

8adf19c

dgquintas merged commit 4eca959 into grpc:master Feb 2, 2018

dgquintas mentioned this pull request Feb 2, 2018

Make RR re-resolve when any of its subchannels fail. #14170

Merged

lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

DNS cooldown mechanism #14228

DNS cooldown mechanism #14228

Conversation

dgquintas commented Jan 30, 2018 • edited

grpc-testing commented Jan 30, 2018

dgquintas Jan 30, 2018

Choose a reason for hiding this comment

grpc-testing commented Jan 30, 2018

grpc-testing commented Jan 30, 2018

markdroth commented Jan 30, 2018

grpc-testing commented Jan 30, 2018

grpc-testing commented Jan 31, 2018

grpc-testing commented Jan 31, 2018

dgquintas commented Jan 31, 2018

grpc-testing commented Jan 31, 2018

grpc-testing commented Jan 31, 2018

grpc-testing commented Jan 31, 2018

grpc-testing commented Jan 31, 2018

grpc-testing commented Jan 31, 2018

grpc-testing commented Jan 31, 2018

dgquintas commented Feb 1, 2018

grpc-testing commented Feb 1, 2018

grpc-testing commented Feb 1, 2018

markdroth commented Feb 1, 2018

dgquintas commented Feb 1, 2018

grpc-testing commented Feb 1, 2018

grpc-testing commented Feb 1, 2018

grpc-testing commented Feb 1, 2018

grpc-testing commented Feb 1, 2018

markdroth commented Feb 1, 2018

dgquintas commented Feb 1, 2018

grpc-testing commented Feb 1, 2018

grpc-testing commented Feb 1, 2018

grpc-testing commented Feb 1, 2018

markdroth commented Feb 1, 2018

grpc-testing commented Feb 1, 2018

grpc-testing commented Feb 1, 2018

grpc-testing commented Feb 1, 2018

grpc-testing commented Feb 2, 2018

grpc-testing commented Feb 2, 2018

grpc-testing commented Feb 2, 2018

dgquintas commented Feb 2, 2018

dgquintas commented Jan 30, 2018 •

edited