Have rls's LRU Cache rely on cleanup process to remove expired entries (

#10400) * Add test for multiple targets with cache expiration.
grpc · Jul 21, 2023 · afa4d6d · afa4d6d
1 parent 419767f
commit afa4d6d
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 16 deletions.
diff --git a/rls/src/main/java/io/grpc/rls/LinkedHashLruCache.java b/rls/src/main/java/io/grpc/rls/LinkedHashLruCache.java
@@ -179,7 +179,6 @@ private SizedValue readInternal(K key) {
     synchronized (lock) {
       SizedValue existing = delegate.get(key);
       if (existing != null && isExpired(key, existing.value, ticker.read())) {
-        invalidate(key, EvictionType.EXPIRED);
         return null;
       }
       return existing;

diff --git a/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java b/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java
@@ -354,12 +354,7 @@ public void get_updatesLbState() throws Exception {
     assertThat(stateCaptor.getAllValues())
         .containsExactly(ConnectivityState.CONNECTING, ConnectivityState.READY);
     Metadata headers = new Metadata();
-    PickResult pickResult = pickerCaptor.getValue().pickSubchannel(
-        new PickSubchannelArgsImpl(
-            TestMethodDescriptors.voidMethod().toBuilder().setFullMethodName("service1/create")
-                .build(),
-            headers,
-            CallOptions.DEFAULT));
+    PickResult pickResult = getPickResultForCreate(pickerCaptor, headers);
     assertThat(pickResult.getStatus().isOk()).isTrue();
     assertThat(pickResult.getSubchannel()).isNotNull();
     assertThat(headers.get(RLS_DATA_KEY)).isEqualTo("header-rls-data-value");
@@ -395,6 +390,57 @@ public void get_updatesLbState() throws Exception {
     assertThat(fakeThrottler.getNumUnthrottled()).isEqualTo(1);
   }
 
+  @Test
+  public void timeout_not_changing_picked_subchannel() throws Exception {
+    setUpRlsLbClient();
+    RouteLookupRequest routeLookupRequest = RouteLookupRequest.create(ImmutableMap.of(
+        "server", "bigtable.googleapis.com", "service-key", "service1", "method-key", "create"));
+    rlsServerImpl.setLookupTable(
+        ImmutableMap.of(
+            routeLookupRequest,
+            RouteLookupResponse.create(
+                ImmutableList.of("primary.cloudbigtable.googleapis.com", "target2", "target3"),
+                "header-rls-data-value")));
+
+    // valid channel
+    CachedRouteLookupResponse resp = getInSyncContext(routeLookupRequest);
+    assertThat(resp.hasData()).isFalse();
+    fakeClock.forwardTime(SERVER_LATENCY_MILLIS, TimeUnit.MILLISECONDS);
+
+    resp = getInSyncContext(routeLookupRequest);
+    assertThat(resp.hasData()).isTrue();
+
+    ArgumentCaptor<SubchannelPicker> pickerCaptor = ArgumentCaptor.forClass(SubchannelPicker.class);
+    ArgumentCaptor<ConnectivityState> stateCaptor =
+        ArgumentCaptor.forClass(ConnectivityState.class);
+    verify(helper, times(4)).updateBalancingState(stateCaptor.capture(), pickerCaptor.capture());
+
+    Metadata headers = new Metadata();
+    PickResult pickResult = getPickResultForCreate(pickerCaptor, headers);
+    assertThat(pickResult.getStatus().isOk()).isTrue();
+    assertThat(pickResult.getSubchannel().toString())
+        .isEqualTo("primary.cloudbigtable.googleapis.com");
+
+    fakeClock.forwardTime(5, TimeUnit.MINUTES);
+    PickResult pickResult2 = getPickResultForCreate(pickerCaptor, headers);
+    assertThat(pickResult2.getSubchannel()).isNull();
+    fakeClock.forwardTime(SERVER_LATENCY_MILLIS, TimeUnit.MILLISECONDS);
+    PickResult pickResult3 = getPickResultForCreate(pickerCaptor, headers);
+    assertThat(pickResult3.getSubchannel()).isNotNull();
+    assertThat(pickResult3.getSubchannel().toString())
+        .isEqualTo(pickResult.getSubchannel().toString());
+  }
+
+  private static PickResult getPickResultForCreate(ArgumentCaptor<SubchannelPicker> pickerCaptor,
+      Metadata headers) {
+    return pickerCaptor.getValue().pickSubchannel(
+        new PickSubchannelArgsImpl(
+            TestMethodDescriptors.voidMethod().toBuilder().setFullMethodName("service1/create")
+                .build(),
+            headers,
+            CallOptions.DEFAULT));
+  }
+
   @Test
   public void get_withAdaptiveThrottler() throws Exception {
     AdaptiveThrottler adaptiveThrottler =
@@ -440,12 +486,7 @@ public void get_withAdaptiveThrottler() throws Exception {
         .updateBalancingState(stateCaptor.capture(), pickerCaptor.capture());
 
     Metadata headers = new Metadata();
-    PickResult pickResult = pickerCaptor.getValue().pickSubchannel(
-        new PickSubchannelArgsImpl(
-            TestMethodDescriptors.voidMethod().toBuilder().setFullMethodName("service1/create")
-                .build(),
-            headers,
-            CallOptions.DEFAULT));
+    PickResult pickResult = getPickResultForCreate(pickerCaptor, headers);
     assertThat(pickResult.getSubchannel()).isNotNull();
     assertThat(headers.get(RLS_DATA_KEY)).isEqualTo("header-rls-data-value");
 
@@ -680,7 +721,8 @@ public PickResult pickSubchannel(PickSubchannelArgs args) {
                 new SubchannelPicker() {
                   @Override
                   public PickResult pickSubchannel(PickSubchannelArgs args) {
-                    return PickResult.withSubchannel(mock(Subchannel.class));
+                    return PickResult.withSubchannel(
+                        mock(Subchannel.class, config.get("target").toString()));
                   }
                 });
           }

diff --git a/rls/src/test/java/io/grpc/rls/LinkedHashLruCacheTest.java b/rls/src/test/java/io/grpc/rls/LinkedHashLruCacheTest.java
@@ -150,13 +150,16 @@ public void eviction_size_shouldEvictAlreadyExpired() {
   }
 
   @Test
-  public void eviction_get_shouldNotReturnAlreadyExpired() {
+  public void eviction_cleanupShouldRemoveAlreadyExpired() {
     for (int i = 1; i <= MAX_SIZE; i++) {
       // last entry is already expired when added
-      cache.cache(i, new Entry("Entry" + i, ticker.read() + MAX_SIZE - i));
+      cache.cache(i, new Entry("Entry" + i,
+          ticker.read() + ((MAX_SIZE - i) * TimeUnit.MINUTES.toNanos(1)) + 1));
     }
 
     assertThat(cache.estimatedSize()).isEqualTo(MAX_SIZE);
+
+    fakeClock.forwardTime(1, TimeUnit.MINUTES);
     assertThat(cache.read(MAX_SIZE)).isNull();
     assertThat(cache.estimatedSize()).isEqualTo(MAX_SIZE - 1);
     verify(evictionListener).onEviction(eq(MAX_SIZE), any(Entry.class), eq(EvictionType.EXPIRED));