core: use exponential backoff for name resolution #4105
Conversation
|
This will also resolve #4028 |
| return; | ||
| } | ||
| if (nameResolverBackoffFuture != null) { | ||
| cancelNameResolverBackoff(); |
There was a problem hiding this comment.
For clarity, could you add an assert nameResolverStarted;? That also helps during auditing.
|
+@zhangkun83 for the NameResolver error+refresh semantic change |
| @@ -402,6 +404,45 @@ public void run() { | |||
| idleTimeoutMillis, TimeUnit.MILLISECONDS); | |||
| } | |||
|
|
|||
| // Run from channelExecutor | |||
| private class NameResolverBackoff implements Runnable { | |||
There was a problem hiding this comment.
nit: this Runnable doesn't actually back off. It is the refresh task that is backed off. Maybe something like "NameResolverBackedOffRetry"
There was a problem hiding this comment.
Done (changed to NameResolverRefresh)
|
|
||
| timer.forwardNanos(RECONNECT_BACKOFF_INTERVAL_NANOS); |
There was a problem hiding this comment.
To verify refresh() is not called sooner than expected, you will need to forward by RECONNECT_BACKOFF_INTERVAL_NANOS - 1 and verify that refresh() is not called, then forward by 1.
| resolver.error = null; | ||
|
|
||
| // For the second attempt, the backoff should occur at RECONNECT_BACKOFF_INTERVAL_NANOS * 2 | ||
| timer.forwardNanos(RECONNECT_BACKOFF_INTERVAL_NANOS); |
There was a problem hiding this comment.
Ditto. The forward time here should be RECONNECT_BACKOFF_INTERVAL_NANOS * 2 - 1, and the next forward should be 1.
|
|
||
| // Verify that the successful resolution reset the backoff policy | ||
| resolver.listener.onError(error); | ||
| timer.forwardNanos(RECONNECT_BACKOFF_INTERVAL_NANOS); |
|
|
||
| assertEquals(1, resolver.refreshCalled); | ||
| verify(mockLoadBalancer, times(2)).handleNameResolutionError(same(error)); | ||
|
|
There was a problem hiding this comment.
To verify that you will not schedule duplicate timers, maybe also call refresh() here, and verify handleNameResolutionError() is called twice, and there is still one timer scheduled.
| assertNotNull(nameResolverBackoff); | ||
| assertFalse(nameResolverBackoff.isCancelled()); | ||
|
|
||
| channel.shutdown(); |
There was a problem hiding this comment.
shutdown() doesn't cancel the timer. delayedTransport termination does. To verify this, you'd need to start a call that stops delayedTransport from terminating.
|
|
||
| @Override public String getServiceAuthority() { | ||
| return expectedUri.getAuthority(); | ||
| } | ||
|
|
||
| @Override public void start(final Listener listener) { | ||
| this.listener = listener; | ||
| if (error != null) { |
There was a problem hiding this comment.
Can we consolidate this with the error simulation in refresh() and put it in resolved()?
There was a problem hiding this comment.
Done. Also made a larger scale refactor to switch to a builder for the FakeNameResolverFactory, as it was confusing sorting through the four different constructors and their different default values.
| private FakeClock.ScheduledTask getNameResolverBackoff() { | ||
| FakeClock.ScheduledTask nameResolverBackoff = null; | ||
| for (FakeClock.ScheduledTask task : timer.getPendingTasks()) { | ||
| if (task.command.toString().contains("NameResolverBackoff")) { |
There was a problem hiding this comment.
You may consider using getPendingTasks(TaskFilter filter) instead.
| assertFalse(task.isDone()); | ||
| nameResolverRefresh = task; | ||
| } | ||
| for (FakeClock.ScheduledTask task : timer.getPendingTasks(NAME_RESOLVER_REFRESH_TASK_FILTER)) { |
There was a problem hiding this comment.
This can be simplified to:
nameResolverRefresh = Iterables.getOnlyElement(timer.getPendingTasks(NAME_RESOLVER_REFRESH_TASK_FILTER));
This addresses #3685.
Drops the fixed 60-second backoff timer from
DnsNameResolver. Instead,ManagedChannelImplwill use its backoff policy to invokeNameResolver.refresh()whenNameResolver.Listener.onErrorsignals that an error occurred during resolution.