[test](fe) Stabilize async cache refresh test#65873
Draft
shuke987 wants to merge 2 commits into
Draft
Conversation
### What problem does this PR solve?\n\nIssue Number: None\n\nThe test used a fixed 2.5 second sleep to wait for a two-second asynchronous refresh, which can observe a legal in-flight refresh under CI scheduling delay. Wait for the existing counter oracle with a bounded timeout instead.\n\n### Release note\n\nNone\n\n### Check List\n\n- [x] Test-only change\n- [x] Target test repeated 10 times\n- [x] Full CacheFactoryTest repeated twice
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review: request changes
I found one P2 test-correctness issue. The new wait observes loader-side progress before the asynchronous refresh is cache-visible, so a legal executor schedule can still make the later expiry assertion flaky. See the inline comment for the concrete execution and fix direction.
Critical checkpoint conclusions:
- Goal and proof: The PR is narrowly intended to stabilize
testAsyncLoadingCacheWithExpireAfterWrite, butcounter == 2does not prove future completion or cache publication, so the stated goal is not fully achieved. The author-reported repeated passes are useful sampling but cannot exclude this ordering. - Scope and clarity: The diff is small and focused: one test import and one wait replacement. Awaitility is already declared and used in FE-core.
- Concurrency and lifecycle: The relevant actors are the test/Awaitility thread, the daemon executor worker, and Caffeine's completion/publication action.
AtomicLongsafely exposes the increment but cannot order actions that occur after it. No locks, static initialization, production lifecycle, or production shared state are changed; the daemon executor's non-shutdown behavior predates this PR. - Configuration, compatibility, persistence, and data paths: No configuration, API/symbol, serialization, FE-BE variable, rolling-upgrade, transaction, persistence, data-write, or cloud/shared-nothing behavior changes.
- Parallel paths and coverage: The unchanged synchronous-cache sleeps are outside this PR's specifically async scope. Exact equality cannot skip 2 before the later expiry access, the fresh executor has capacity, and the 10-second timeout introduces no separate supported defect. No generated result file applies.
- Performance and observability: The polling wait has no production impact and can reduce test duration; production observability is not applicable.
Validation and completion: I reviewed the authoritative one-file diff and relevant loader/cache/executor paths, deduplicated against both the supplied and live GitHub review state, and completed three review rounds. The final full-review and risk-focused agents both returned NO_NEW_VALUABLE_FINDINGS against this exact comment set. Per the runner contract, I did not run builds or tests; git diff --check is clean, CheckStyle passed, and FE UT/compile were still pending at submission time. No additional user focus was provided. The review itself is complete with one inline issue and no unresolved candidate points.
| // 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); |
Contributor
There was a problem hiding this 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.
### What problem does this PR solve?\n\nIssue Number: None\n\nThe review identified that the loader counter can advance before the refreshed future is published to Caffeine. Wait until the cache exposes a different completed future before advancing fake time.\n\n### Release note\n\nNone\n\n### Check List\n\n- [x] Target test repeated 10 times\n- [x] Full CacheFactoryTest repeated twice
Contributor
TPC-H: Total hot run time: 29396 ms |
Contributor
TPC-DS: Total hot run time: 177200 ms |
Contributor
ClickBench: Total hot run time: 25.06 s |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary
No new inline issues were found on head 25e9e9a117be2cfdc254bcec754209c7d51c5374. The existing P2 publication-order concern is addressed: the test retains the pre-refresh cached future and waits until the cache exposes a different completed future before advancing FakeTicker, so loader-side progress alone cannot satisfy the wait.
Critical checkpoint conclusions:
- Goal and proof: The patch accomplishes its stated one-test goal by replacing a fixed 2.5-second scheduling assumption with a bounded cache-visible condition while preserving assertions for stale-value refresh, exactly one refresh load, and expiry-triggered reload. The PR reports targeted and repeated unit-test passes; those tests were not rerun in this review-only runner.
- Scope and clarity: The source change is small, focused, and uses the existing Awaitility test dependency. The predicate and comment explain the required publication boundary. Non-blocking metadata note: the PR description still describes the superseded
counter == 2predicate and could be updated to match the current head. - Concurrency: The relevant actors are the test/Awaitility polling thread and the configured executor's cache-loader thread.
BasicAsyncCacheLoaderreturns a distinct future, and Caffeine retains the old mapping while refresh is in flight; repeated polls neither publish early nor start another refresh. No project lock or shared non-atomic mutable state is added. - Lifecycle and static initialization: No production resource ownership, shutdown order, static initialization, or cross-TU lifecycle is changed. The pre-existing daemon executor lifecycle is unchanged.
- Parallel paths and special conditions: The two older synchronous
LoadingCachetests still contain fixed waits, but those waits are unchanged, outside the authoritative diff, and outside the PR's explicit single-method goal; they are a separate cleanup opportunity rather than a regression in this patch. The new identity-plus-completion condition fails with a bounded timeout if publication never occurs. - Tests and expected results: No generated result files are involved. The changed test continues to cover the old value during refresh, refresh completion/counter behavior, and expiry after publication. GitHub CheckStyle is green at this head. The external
check_coverage_fe (Coverage)status currently reports a TeamCity failure without public diagnostic detail and remains subject to the normal merge gate. - Configuration, compatibility, persistence, transactions/data writes, FE-BE variables, and observability: Not applicable; this is test-only and changes no runtime configuration, format/protocol, persisted state, data path, logging, or metrics.
- Performance and memory: The bounded polling performs a single cache lookup per interval only in this unit test; there is no production hot-path or memory-accounting impact.
- User focus: No additional user-provided focus was supplied.
The full-review and timing-focused passes converged with NO_NEW_VALUABLE_FINDINGS; all suspicious points have evidence-based conclusions, the prior thread is not duplicated, and the final inline-comment set is empty.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
CacheFactoryTest.testAsyncLoadingCacheWithExpireAfterWritewaited a fixed 2.5 seconds for an asynchronous refresh whose loader deliberately sleeps for 2 seconds. Executor scheduling, JVM pauses, or an oversleep of more than the remaining 500 ms can leave the legal background refresh in flight and make the counter assertion flaky.Replace the wall-clock sleep with a bounded Awaitility wait for the existing
counter == 2semantic oracle. The assertions that refresh immediately returns the old cached value and that expiry performs a new load remain unchanged.Release note
None
Check List (For Author)
Validation:
./run-fe-ut.sh --run org.apache.doris.common.CacheFactoryTest#testAsyncLoadingCacheWithExpireAfterWrite: passedExact method repeated 10 times: 10/10 passed
Full
CacheFactoryTestrepeated twice: 2/2 passed, 6 tests per roundExact method after rebasing onto the latest
master: passedBehavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)