github-actions[bot] commented on code in PR #65873:
URL: https://github.com/apache/doris/pull/65873#discussion_r3628256482
##########
fe/fe-core/src/test/java/org/apache/doris/common/CacheFactoryTest.java:
##########
@@ -237,8 +238,8 @@ public void testAsyncLoadingCacheWithExpireAfterWrite()
throws InterruptedExcept
// refresh in background, so still get value1
Assertions.assertTrue(futureValue.isDone());
Assertions.assertEquals("value1", futureValue.get().get().getValue());
- // sleep longer to wait for refresh
- Thread.sleep(2500);
+ // Wait for the refresh triggered above instead of relying on executor
scheduling time.
+ Awaitility.await().atMost(10, TimeUnit.SECONDS).until(() ->
counter.get() == 2);
Review Comment:
[P2] Wait for the refresh publication, not just the counter
`createValue` increments `counter` before returning, so this predicate can
succeed before `BasicAsyncCacheLoader` completes the reload future and before
Caffeine replaces the old cached future. If the loader is descheduled after the
increment, line 243 still reads the old completed future; the test can then
advance the `FakeTicker` by 61 seconds, and if the refresh publishes at the
advanced fake time, line 249 sees a fresh completed entry and
`assertFalse(futureValue.isDone())` still flakes. The `get()` at line 243 does
not wait for the refresh because Caffeine returns the old value during refresh.
Please wait for cache-visible publication (for example, retain the pre-refresh
cached future and await until the cache holds a different future), then assert
the counter before advancing fake time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
