11243::RLS cleanups #12085

vimanikag · 2025-05-22T06:21:34Z

addressed the review comments provided on #11243

adding junit's for cache.fitToLimit()
updated the comments as per the suggestions on rls: Make LinkedHashLruCache non-threadsafe #11203
moved out the decrement estimatedSizeBytes logic to specific to the each required methods

ejona86 · 2025-05-27T21:58:53Z

rls/src/test/java/io/grpc/rls/LinkedHashLruCacheTest.java

+    cache.cache(3, entry3);
+
+    assertThat(cache.estimatedSize()).isEqualTo(2);
+    cache.setEstimatedSizeBytes(7);


What's the point of this? This method shouldn't exist, as it corrupts the state.

The reason for using the setEstimatedSizeBytes()/setEstimatedMaxSizeBytes() is due to we are internally invoking the fitToLimit() in resize() hence I did not see the scenario which satisfy the below condition in fitToLimit()

if (estimatedSizeBytes.get() <= estimatedMaxSizeBytes) {

Most of the cases above condition is false hence no clean-up logic invoked and we don't have any direct method to alter the estimatedSizeBytes to satisfy the above condition.

we have one possibility to use resize() to alter the estimatedSizeBytes but no difference if we invoke resize() in fitToLimit junit's to alter the estimatedSizeBytes.

To change estimatedSizeBytes you add entries. To change estimatedMaxSizeBytes you call resize().

Addressed the review comments and fallowed the suggestion to setEstimatedSizeBytes for adjusting estimatedSizeBytes and used resize() for changing the estimatedMaxSizeBytes

In no way did I say to use setEstimatedSizeBytes. I've been arguing to delete it. I had said to change the estimated size you add entries.

Addressed the review comments and used the entry to full fill the requirements instead of setEstimatedSizeBytes

ejona86 · 2025-05-27T22:01:03Z

rls/src/test/java/io/grpc/rls/LinkedHashLruCacheTest.java

+    cache.cache(3, entry3);
+
+    assertThat(cache.estimatedSize()).isEqualTo(2);
+    cache.setEstimatedMaxSizeBytes(2);


Why make this method? You can construct the cache with a different value or you can use resize(). We want to use the public APIs, not dig in and make random changes that invalidate the current state.

Yeah , We can use the construct cache with different size and I have followed the same in the test case to reset the estimatedMaxSizeBytes

I have used setEstimatedMaxSizeBytes() here as I have added this new method to cover the junit's.

as We have alternative to setEstimatedMaxSizeBytes , I will avoid this new method and update the test cases accordingly.

I didn't understand any of that, as the first sentence just sounds wrong and the second is circular. Just remove the mutating methods.

fallowed the above suggestions and updated the testFitToLimitUsingReSize and used resize() for changing the estimatedMaxSizeBytes and updated the test case accordingly

rls/src/main/java/io/grpc/rls/LinkedHashLruCache.java

ejona86 · 2025-05-27T22:02:36Z

rls/src/main/java/io/grpc/rls/LinkedHashLruCache.java

@@ -136,6 +136,8 @@ 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);


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

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.

Make a new private method, not in the interface.

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

ejona86 · 2025-05-27T22:03:01Z

rls/src/main/java/io/grpc/rls/LinkedHashLruCache.java

@@ -294,7 +304,7 @@ private final class SizeHandlingEvictionListener implements EvictionListener<K,

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


Clean this up, otherwise, what's the point?

Yes , it's missed testing different case and I will clean-up this code

vimanikag added 3 commits May 22, 2025 06:17

11243::RLS cleanups test

84c05ba

11243::RLS cleanups test

6495235

11243::RLS cleanups test

901ce2b

vimanikag marked this pull request as ready for review May 26, 2025 13:51

ejona86 reviewed May 27, 2025

View reviewed changes

vimanikag added 3 commits May 29, 2025 06:13

11243 : RLS cleanups

afdbe1e

11243::RLS cleanups

b1665ff

11243::RLS cleanups

44a715f

vimanikag requested a review from ejona86 June 9, 2025 14:10

11243::RLS cleanups #12085

Are you sure you want to change the base?

11243::RLS cleanups #12085

Conversation

vimanikag commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vimanikag May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vimanikag commented May 22, 2025 •

edited

Loading

vimanikag May 28, 2025 •

edited

Loading