fix(QTDI-2567) prevent CacheMap eviction issue#1181
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a runtime exception in MapCache.evictIfNeeded() when trimming concurrent caches used by component-server resources, and adds unit tests to validate eviction behavior.
Changes:
- Fix eviction logic to remove entries safely via iterator
next()+remove(), and treatmaxSize <= 0as “clear cache”. - Add JUnit 5 parameterized tests covering emptying, trimming, and no-op scenarios for
evictIfNeeded(). - Minor comment typo fixes in
MapCache.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| component-server-parent/component-server/src/main/java/org/talend/sdk/component/server/lang/MapCache.java | Fixes eviction implementation to avoid iterator misuse exceptions when enforcing max cache size. |
| component-server-parent/component-server/src/test/java/org/talend/sdk/component/server/lang/MapCacheTest.java | Adds unit tests for eviction behavior (empty, trimmed, unchanged). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final Iterator<A> it = cache.keySet().iterator(); | ||
| int excess = cache.size() - maxSize; | ||
| while (excess-- > 0 && it.hasNext()) { | ||
| it.next(); | ||
| it.remove(); |
There was a problem hiding this comment.
java.util.Map is still imported in this class, but after switching to a keySet() iterator the type is no longer referenced. In Java, unused imports are compilation errors, so this will break the build; please remove the unused Map import (and any other now-unused imports).
| return Stream.of( | ||
| Arguments.of(new ConcurrentHashMap<>(Map.of("1", "2", "3", "4", "5", "6")), 1), | ||
| Arguments.of(new ConcurrentHashMap<>(Map.of("1", "2", "3", "4", "5", "6")), 2) | ||
| ); |
There was a problem hiding this comment.
Map.of("1","2","3","4","5","6") creates a map with 3 entries (pairs), not 6. This makes the eviction test much weaker than intended and can mask regressions; please build a map with 6 entries (e.g., Map.ofEntries(...) or explicit puts) so the starting size is actually > maxSize by a larger margin.
| return Stream.of( | ||
| Arguments.of(new ConcurrentHashMap<>(Map.of("1", "2", "3", "4", "5", "6")), 3), | ||
| Arguments.of(new ConcurrentHashMap<>(Map.of("1", "2", "3", "4", "5", "6")), 4) | ||
| ); |
There was a problem hiding this comment.
Same issue here: Map.of("1","2","3","4","5","6") only creates 3 entries. If the intent is to validate that the cache is not modified when maxSize is comfortably above the current size, consider using a map with 6 actual entries and choosing maxSize accordingly to make this test assert what its name suggests.
Requirements
Why this PR is needed?
Component-server maintains two maps with a size limit. The code that should take care of this limit throws an exception when called
What does this PR adds (design/code thoughts)?
It prevents the exception, and a test case is also provided.