Skip to content

11243::RLS cleanups #12085

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

11243::RLS cleanups #12085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
84c05ba
11243::RLS cleanups test
vimanikag May 22, 2025
6495235
11243::RLS cleanups test
vimanikag May 26, 2025
901ce2b
11243::RLS cleanups test
vimanikag May 26, 2025
afdbe1e
11243 : RLS cleanups
vimanikag May 29, 2025
b1665ff
11243::RLS cleanups
vimanikag Jun 3, 2025
44a715f
11243::RLS cleanups
vimanikag Jun 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions rls/src/main/java/io/grpc/rls/LinkedHashLruCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ public final V cache(K key, V value) {
existing = delegate.put(key, new SizedValue(size, value));
if (existing != null) {
evictionListener.onEviction(key, existing, EvictionType.REPLACED);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest making a private onEviction() method that calls the evictionListener and updates the estimatedSizeBytes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't reduce the visibility to private for onEviction() as it's overridden method and it's defined as public implicitly in EvictionListener interface , please let me know Your thoughts on this to take it forward.

Screenshot 2025-05-28 11 32 12 AM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make a new private method, not in the interface.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried the above suggested private method changes and observed the findings below. Hence I have followed the new approach , please review it and let me know Your thoughts on the current approach :

Observations with private onEvaction :

I have tried adding new private method to handle the Eviction type and avoided the current SizeHandlingEvictionListener onEviction code and observed it's casing the build failure due to CachingRlsLbClient.close internally using LinkedHashLruCache.close and most of the use cases are impacted with ClassCastException issues due to LinkedHashLruCache onEvaction has avoided(removed) as is not used.

CachingRlsLbClientTest > rls_withCustomRlsChannelServiceConfig FAILED java.lang.ClassCastException: class io.grpc.rls.LinkedHashLruCache$SizedValue cannot be cast to class io.grpc.rls.CachingRlsLbClient$CacheEntry (io.grpc.rls.LinkedHashLruCache$SizedValue and io.grpc.rls.CachingRlsLbClient$CacheEntry are in unnamed module of loader 'app') at io.grpc.rls.CachingRlsLbClient$AutoCleaningEvictionListener.onEviction(CachingRlsLbClient.java:854) at io.grpc.rls.LinkedHashLruCache.onEviction(LinkedHashLruCache.java:351) at io.grpc.rls.LinkedHashLruCache.invalidateAll(LinkedHashLruCache.java:190) at io.grpc.rls.LinkedHashLruCache.close(LinkedHashLruCache.java:288) at io.grpc.rls.CachingRlsLbClient.close(CachingRlsLbClient.java:356) at io.grpc.rls.CachingRlsLbClientTest.tearDown(CachingRlsLbClientTest.java:194)

We are getting above issue due to current LinkedHashLruCache onEvaction implementation has removed as it's not using with private onEvaction method and it's using other child class (CachingRlsLbClient) onEvaction implementation it's casing for ClassCastException issue while casting SizedValue with CacheEntry

estimatedSizeBytes -= existing.size;
}
return existing == null ? null : existing.value;
}
Expand Down Expand Up @@ -175,6 +176,7 @@ private V invalidate(K key, EvictionType cause) {
SizedValue existing = delegate.remove(key);
if (existing != null) {
evictionListener.onEviction(key, existing, cause);
estimatedSizeBytes -= existing.size;
}
return existing == null ? null : existing.value;
}
Expand All @@ -186,6 +188,7 @@ public final void invalidateAll() {
Map.Entry<K, SizedValue> entry = iterator.next();
if (entry.getValue() != null) {
evictionListener.onEviction(entry.getKey(), entry.getValue(), EvictionType.EXPLICIT);
estimatedSizeBytes -= entry.getValue().size;
}
iterator.remove();
}
Expand Down Expand Up @@ -215,7 +218,7 @@ public final List<V> values() {
protected final boolean fitToLimit() {
boolean removedAnyUnexpired = false;
if (estimatedSizeBytes <= estimatedMaxSizeBytes) {
// new size is larger no need to do cleanup
// there is space available so no need to do cleanup
return false;
}
// cleanup expired entries
Expand All @@ -230,8 +233,9 @@ protected final boolean fitToLimit() {
break; // Violates some constraint like minimum age so stop our cleanup
}
lruIter.remove();
// eviction listener will update the estimatedSizeBytes
// eviction listener is used to track the eviction type
evictionListener.onEviction(entry.getKey(), entry.getValue(), EvictionType.SIZE);
estimatedSizeBytes -= entry.getValue().size;
removedAnyUnexpired = true;
}
return removedAnyUnexpired;
Expand Down Expand Up @@ -271,6 +275,7 @@ private boolean cleanupExpiredEntries(int maxExpiredEntries, long now) {
if (isExpired(entry.getKey(), entry.getValue().value, now)) {
lruIter.remove();
evictionListener.onEviction(entry.getKey(), entry.getValue(), EvictionType.EXPIRED);
estimatedSizeBytes -= entry.getValue().size;
removedAny = true;
maxExpiredEntries--;
}
Expand All @@ -294,7 +299,10 @@ private final class SizeHandlingEvictionListener implements EvictionListener<K,

@Override
public void onEviction(K key, SizedValue value, EvictionType cause) {
estimatedSizeBytes -= value.size;
handleEvictionType(key,value,cause);
}

private void handleEvictionType(K key, SizedValue value, EvictionType cause) {
if (delegate != null) {
delegate.onEviction(key, value.value, cause);
}
Expand Down
41 changes: 39 additions & 2 deletions rls/src/test/java/io/grpc/rls/LinkedHashLruCacheTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ public void eviction_explicit() {
cache.cache(1, survivor);

assertThat(cache.invalidate(0)).isEqualTo(toBeEvicted);

verify(evictionListener).onEviction(0, toBeEvicted, EvictionType.EXPLICIT);
}

Expand All @@ -143,7 +142,7 @@ public void eviction_size_shouldEvictAlreadyExpired() {

// should remove MAX_SIZE-1 instead of MAX_SIZE because MAX_SIZE is accessed later
verify(evictionListener)
.onEviction(eq(MAX_SIZE - 1), any(Entry.class), eq(EvictionType.EXPIRED));
.onEviction(eq(MAX_SIZE - 1), any(Entry.class), eq(EvictionType.EXPIRED));
assertThat(cache.estimatedSize()).isEqualTo(MAX_SIZE);
}

Expand Down Expand Up @@ -266,4 +265,42 @@ public int hashCode() {
return Objects.hash(value, expireTime);
}
}

@Test
public void testFitToLimitUsingReSize() {
Entry entry1 = new Entry("Entry1", ticker.read() + 10,4);
Entry entry2 = new Entry("Entry2", ticker.read() + 20,2);
Entry entry3 = new Entry("Entry3", ticker.read() + 30,1);

cache.cache(1, entry1);
cache.cache(2, entry2);
cache.cache(3, entry3);

assertThat(cache.estimatedSize()).isEqualTo(2);
cache.resize(1);

assertThat(cache.estimatedMaxSizeBytes()).isEqualTo(1);
assertThat(cache.fitToLimit()).isEqualTo(false);
assertThat(cache.estimatedSizeBytes()).isEqualTo(1);
}

@Test
public void testFitToLimitWithEntry() {
Entry entry1 = new Entry("Entry1", ticker.read() + 10,4);
Entry entry2 = new Entry("Entry2", ticker.read() + 20,2);
Entry entry3 = new Entry("Entry3", ticker.read() + 30,1);
Entry entry4 = new Entry("Entry4", ticker.read() + 20,2);

cache.cache(1, entry1);
cache.cache(2, entry2);
cache.cache(3, entry3);

assertThat(cache.estimatedSize()).isEqualTo(2);
cache.cache(4, entry4);

assertThat(cache.estimatedSize()).isEqualTo(3);
assertThat(cache.estimatedMaxSizeBytes()).isEqualTo(5);
assertThat(cache.fitToLimit()).isEqualTo(false);
assertThat(cache.estimatedSizeBytes()).isEqualTo(5);
}
}