From 20244b897492854bacb3e77bb71dc84e698e42ae Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Fri, 30 Oct 2020 17:35:55 -0700 Subject: [PATCH] Only reschedule the initial resource fetch timer for unresolved resources at ADS stream restart. --- .../java/io/grpc/xds/ClientXdsClient.java | 4 ++- .../java/io/grpc/xds/ClientXdsClientTest.java | 25 ++++++++++++++++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java index 5c1d8fcb94a..8ddbff0d550 100644 --- a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java +++ b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java @@ -754,8 +754,10 @@ void removeWatcher(ResourceWatcher watcher) { watchers.remove(watcher); } - // FIXME(chengyuanzhang): should only restart timer if the resource is still unresolved. void restartTimer() { + if (data != null || absent) { // resource already resolved + return; + } class ResourceNotFound implements Runnable { @Override public void run() { diff --git a/xds/src/test/java/io/grpc/xds/ClientXdsClientTest.java b/xds/src/test/java/io/grpc/xds/ClientXdsClientTest.java index e1862e53617..d8430245c5c 100644 --- a/xds/src/test/java/io/grpc/xds/ClientXdsClientTest.java +++ b/xds/src/test/java/io/grpc/xds/ClientXdsClientTest.java @@ -1463,7 +1463,7 @@ public void streamClosedAndRetryRaceWithAddRemoveWatchers() { } @Test - public void streamClosedAndRetryRestartResourceInitialFetchTimers() { + public void streamClosedAndRetryRestartsResourceInitialFetchTimerForUnresolvedResources() { xdsClient.watchLdsResource(LDS_RESOURCE, ldsResourceWatcher); xdsClient.watchRdsResource(RDS_RESOURCE, rdsResourceWatcher); xdsClient.watchCdsResource(CDS_RESOURCE, cdsResourceWatcher); @@ -1477,15 +1477,32 @@ public void streamClosedAndRetryRestartResourceInitialFetchTimers() { Iterables.getOnlyElement(fakeClock.getPendingTasks(CDS_RESOURCE_FETCH_TIMEOUT_TASK_FILTER)); ScheduledTask edsResourceTimeout = Iterables.getOnlyElement(fakeClock.getPendingTasks(EDS_RESOURCE_FETCH_TIMEOUT_TASK_FILTER)); - call.responseObserver.onError(Status.UNAVAILABLE.asException()); + + List listeners = ImmutableList.of( + Any.pack(buildListener(LDS_RESOURCE, + Any.pack( + HttpConnectionManager.newBuilder() + .setRouteConfig(buildRouteConfiguration("do not care", buildVirtualHosts(2))) + .build())))); + DiscoveryResponse response = + buildDiscoveryResponse("0", listeners, ResourceType.LDS.typeUrl(), "0000"); + call.responseObserver.onNext(response); assertThat(ldsResourceTimeout.isCancelled()).isTrue(); + + List routeConfigs = + ImmutableList.of(Any.pack(buildRouteConfiguration(RDS_RESOURCE, buildVirtualHosts(2)))); + response = + buildDiscoveryResponse("0", routeConfigs, ResourceType.RDS.typeUrl(), "0000"); + call.responseObserver.onNext(response); assertThat(rdsResourceTimeout.isCancelled()).isTrue(); + + call.responseObserver.onError(Status.UNAVAILABLE.asException()); assertThat(cdsResourceTimeout.isCancelled()).isTrue(); assertThat(edsResourceTimeout.isCancelled()).isTrue(); fakeClock.forwardNanos(10L); - assertThat(fakeClock.getPendingTasks(LDS_RESOURCE_FETCH_TIMEOUT_TASK_FILTER)).hasSize(1); - assertThat(fakeClock.getPendingTasks(RDS_RESOURCE_FETCH_TIMEOUT_TASK_FILTER)).hasSize(1); + assertThat(fakeClock.getPendingTasks(LDS_RESOURCE_FETCH_TIMEOUT_TASK_FILTER)).hasSize(0); + assertThat(fakeClock.getPendingTasks(RDS_RESOURCE_FETCH_TIMEOUT_TASK_FILTER)).hasSize(0); assertThat(fakeClock.getPendingTasks(CDS_RESOURCE_FETCH_TIMEOUT_TASK_FILTER)).hasSize(1); assertThat(fakeClock.getPendingTasks(EDS_RESOURCE_FETCH_TIMEOUT_TASK_FILTER)).hasSize(1); }