xds: float LRU cache across interceptors #11992
Conversation
| return; | ||
| } | ||
| LinkedHashMap<K, V> newCache = createEvictingMap(newSize); | ||
| newCache.putAll(cache); |
There was a problem hiding this comment.
Because maxSize is only updated after the map is created, this map will contain all the entries from cache.
There was a problem hiding this comment.
createEvictingMap is creating an empty LinkedHashMap. There are separator constructors
LinkedHashMap(int initialCapacity, float loadFactor, boolean accessOrder)
LinkedHashMap(Map<? extends K,? extends V> m)
but none that both take an old map and specify the initialCapacity.
| return; | ||
| } | ||
| LinkedHashMap<K, V> newCache = createEvictingMap(newSize); | ||
| newCache.putAll(cache); |
There was a problem hiding this comment.
createEvictingMap is creating an empty LinkedHashMap. There are separator constructors
LinkedHashMap(int initialCapacity, float loadFactor, boolean accessOrder)
LinkedHashMap(Map<? extends K,? extends V> m)
but none that both take an old map and specify the initialCapacity.
| } | ||
|
|
||
| V getOrInsert(K key, Function<K, V> create) { | ||
| return cache.computeIfAbsent(key, create); | ||
| } | ||
|
|
||
| private void resizeCache(int newSize) { | ||
| if (newSize >= maxSize) { |
There was a problem hiding this comment.
Can't understand this. I thought if you get a bigger new size you should resize, and if you get an equal or smaller new size only that should be a no-op because decreasing cache size doesn't make since when another filter instance created a bigger cache.
There was a problem hiding this comment.
I agree with your point but well, cache_size is configuration coming externally. We should adhere to the configuration. If the cache size decreases, then we can't use the normal code to discard entries, as that only discards a single entry. So without special code, the configuration wouldn't be observed.
It is generally really bad to not observe configuration updates, from a testing/debugging perspective. You do a config push and see behavior X, but then after restarting you see behavior Y. It can be really hard to track down the bugs.
The cache size doesn't change often, so the performance hit would be temporary (although there are still issues with that). It's also much easier to figure out what happened. If you keep the old cache, then someone may be debugging something and it matters what happened a month ago.
I discussed this with Eric offline on the similar lines and understood this POV.
| CallOptions capturedOptions2 = callOptionsCaptor.getAllValues().get(1); | ||
| assertNotNull(capturedOptions2.getCredentials()); | ||
|
|
||
| assertSame(capturedOptions1.getCredentials(), capturedOptions2.getCredentials()); |
There was a problem hiding this comment.
Cached value is tested but not eviction behavior. With cache max size of 1, you need to test using a cluster resource with a 2nd audience string and get the credentials and assert the behavior for the 1st audience string's call credentials getting evicted.
filter max size 2
audience-1 : call-credentials-for-audience-1
filter max size 1
audience-1: call-credentials-1-for-audience-1 (same instance)
audience-2: call-credentials-1-for-audience-2 (causes eviction of call-credentials-1-for-audience-1)
audience-1: call-credentials-2-for-audience-1 (new instance)
| CallOptions options4 = captor.getValue(); | ||
| assertNotNull(options4.getCredentials()); | ||
|
|
||
| assertSame(options1.getCredentials(), options2.getCredentials()); |
There was a problem hiding this comment.
These 3 are the only assertions needed in this test. Remove all other assertions as they are asserted in their respective tests and behavior driven unit tests' guidance calls for asserting only the behavior tested by that particular unit test.
Also to make the test more understandable move these assertions to right after the credentials are obtained and are available for assertion.
There was a problem hiding this comment.
All asserts should have new line before and after?
There was a problem hiding this comment.
Since we have assertions to do after obtaining call credentials obtained using interceptor 2, 3 and 4 we can think of it as a repeated arrange-act-assert structure suggested for unit tests, so yes there will be new lines before and after
No description provided.