From afa4d6dac815f4c77612103da0b0d0e4e7457d89 Mon Sep 17 00:00:00 2001 From: Larry Safran Date: Fri, 21 Jul 2023 12:12:19 -0700 Subject: [PATCH] Have rls's LRU Cache rely on cleanup process to remove expired entries (#10400) * Add test for multiple targets with cache expiration. --- .../java/io/grpc/rls/LinkedHashLruCache.java | 1 - .../io/grpc/rls/CachingRlsLbClientTest.java | 68 +++++++++++++++---- .../io/grpc/rls/LinkedHashLruCacheTest.java | 7 +- 3 files changed, 60 insertions(+), 16 deletions(-) diff --git a/rls/src/main/java/io/grpc/rls/LinkedHashLruCache.java b/rls/src/main/java/io/grpc/rls/LinkedHashLruCache.java index 8baba0bb824..5a4a2dab452 100644 --- 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 index 120a486dec6..51b934e13b7 100644 --- 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 pickerCaptor = ArgumentCaptor.forClass(SubchannelPicker.class); + ArgumentCaptor 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 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 index 19b3a012151..a31f58f5365 100644 --- 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));