Address flaky IdTests by adding cache retry #3982
Conversation
ddanielr
left a comment
ddanielr
left a comment
There was a problem hiding this comment.
Would there be a reason why we might want to keep the # of retries configurable?
core/src/test/java/org/apache/accumulo/core/util/GuavaCacheCounterUtil.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/accumulo/core/util/GuavaCacheCounterUtil.java
Outdated
Show resolved
Hide resolved
| long initialSize = cacheCount(NamespaceId.cache); | ||
| NamespaceId nsId = NamespaceId.of(namespaceString); | ||
| assertEquals(initialSize + 1, cacheCount()); | ||
| assertCacheCountEquals(initialSize + 1, NamespaceId.cache); |
There was a problem hiding this comment.
If this is equivalent, then this seems simpler.
Wait.waitFor(() -> NamespaceId.cache.asMap().entrySet().stream().count() == initialSize + 1);
There was a problem hiding this comment.
The one issue with using Wait.waitFor() is that it currently is in the test module. Adding the test module causes a circular dependency error - so Wait.waitFor would need to be relocated somewhere common. The Wait.waitFor does not have junit / test dependencies, so relocating it to core should not be an issue. Was just not sure we wanted to do that.
There was a problem hiding this comment.
The Wait.waitFor does not have junit / test dependencies, so relocating it to core should not be an issue. Was just not sure we wanted to do that.
We could just add the Wait class methods to the UtilWaitThread class...
| * get the actual cache size | ||
| */ | ||
| public static long cacheCount(final Cache<?,?> cache) { | ||
| return cache.asMap().entrySet().stream().count(); |
There was a problem hiding this comment.
| return cache.asMap().entrySet().stream().count(); | |
| return cache.asMap().entrySet().size(); |
There was a problem hiding this comment.
The size method was explicitly avoided (as noted in the comment) because of the way guava handles size() with deleted / gc eligible entries
There was a problem hiding this comment.
Oh gotcha, I was thinking that applies to cache.size() as opposed to cache.asMap().entrySet().size().
| received); | ||
| UtilWaitThread.sleep(delayMills); | ||
| } | ||
| assertEquals(expected, received, "expected cache entry count did not match expected"); |
There was a problem hiding this comment.
| assertEquals(expected, received, "expected cache entry count did not match expected"); | |
| assertEquals(expected, received, "actual cache entry count did not match expected"); |
5 participants
The NamespaceIdTest and TableIdTest have been occasionally failing in github actions (and possible Jenkins runs). This change adds a retry to allow cache background clean-up occur that may be effecting the cache entry count. (It is unclear why these tests seemed to have stared failing only recently.)