[fix](fe) Fix stale timestamp in CatalogRecycleBin erase daemon#63310
Open
heguanhui wants to merge 1 commit into
Open
[fix](fe) Fix stale timestamp in CatalogRecycleBin erase daemon#63310heguanhui wants to merge 1 commit into
heguanhui wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
rub buildall |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31429 ms |
Contributor
TPC-DS: Total hot run time: 169884 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
/review |
1 similar comment
Contributor
|
/review |
heguanhui
force-pushed
the
fix/recycle-bin-stale-starttime
branch
2 times, most recently
from
6775e09 to
3a9fdae
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
heguanhui
force-pushed
the
fix/recycle-bin-stale-starttime
branch
from
3a9fdae to
621a300
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29041 ms |
Contributor
TPC-DS: Total hot run time: 168029 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
yx-keith
reviewed
Jun 14, 2026
yx-keith
left a comment
yx-keith
left a comment
Contributor
There was a problem hiding this comment.
The core change is reasonable and low‑risk: calling System.currentTimeMillis() per erase stage instead of once is correct, and since isExpire uses latency = currentTimeMs - recycleTime, a fresher (later) timestamp only makes more items eligible — it doesn't break the partition → table → db ordering invariant (partitions are already processed first). A few things I'd like to see addressed before this goes in:
- The injected clock is unused and adds complexity. You added LongSupplier clock + @VisibleForTesting setClock(), but none of the tests actually call setClock() — they reproduce timing via subclassing + Thread.sleep instead. So the clock abstraction is effectively dead code here. Please pick one direction:
Either use setClock() in the tests (fast, deterministic, no sleeps), or
Drop the clock entirely and just call System.currentTimeMillis() three times — which is exactly what the PR description's "After" snippet shows. Right now the description and the implementation disagree.
2. The tests rely on real Thread.sleep (~6s each, ~24s total). This noticeably slows down FE UT and is the kind of wall‑clock‑timing test the project generally avoids. Injecting the clock (point 1) would let you advance time deterministically and remove all the sleeps.
-
testOldCode* tests re‑implement the bug and assert it's buggy. BuggyRecycleBin overrides runAfterCatalogReady() with the old shared‑timestamp logic, so these tests exercise the mock, not production code. They add ~12s of sleep for no real coverage — suggest removing them and keeping only the "new code" assertions.
-
The partition/table assertions are vacuous. The tests only recycleDatabase(...) — no table or partition is ever recycled. isRecyclePartition/isRecycleTable are isRecycleDatabase(dbId) || ..., so those assertions pass purely off the database's state; the only thing actually verified is database erase timing. The test names (...TableDbWork) are misleading. Please either recycle a real table+partition so the assertions mean something, or scope the test/names to what's actually covered.
-
Unrelated formatting churn. The diff reindents closing parens (16 → 8 spaces) across ~17 existing test methods. Please revert these to keep the diff minimal.
On impact: worth noting the practical window is small — the daemon re‑runs every catalog_recycle_bin_interval_ms (30s default) and catalog_trash_expire_second defaults to 1 day, so missed items are picked up on the next cycle. The fix is a correct nicety, but the PR description ("5 minutes → skipped") overstates a persistent effect; consider toning it down.
Suggested shape: use setClock() to drive time, drop Thread.sleep and the testOldCode* cases, keep one test that recycles a real partition+table+db and verifies they're erased under the fixed path, and revert the formatting changes.
heguanhui
force-pushed
the
fix/recycle-bin-stale-starttime
branch
2 times, most recently
from
11cb708 to
a15360e
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28825 ms |
Contributor
TPC-DS: Total hot run time: 168866 ms |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28297 ms |
Contributor
TPC-DS: Total hot run time: 168965 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
heguanhui
force-pushed
the
fix/recycle-bin-stale-starttime
branch
from
a15360e to
cf634b8
Compare
Contributor
Author
|
run buildall |
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
4 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: close #xxx
Problem Summary:
CatalogRecycleBin.runAfterCatalogReady()capturesSystem.currentTimeMillis()once at the beginning of the method and shares this timestamp acrosserasePartition(),eraseTable(), anderaseDatabase(). Since each erase operation can take significant time (I/O, log writes, lock acquisition), the later methods use a stale timestamp for expiration checks, causing delayed cleanup of tables and databases.For example, if
erasePartitiontakes 5 minutes,eraseTableanderaseDatabasewill use a timestamp that is 5 minutes old, potentially skipping items that became eligible for cleanup during that period.Fix
Each erase method now gets its own fresh
System.currentTimeMillis()call, ensuring accurate expiration checks.Before:
After:
Release note
Fix delayed cleanup of tables and databases in CatalogRecycleBin when partition erasure takes significant time
Check List (For Author)
testEraseUsesFreshCurrentTimeinCatalogRecycleBinTestthat creates a table with 1000 partitions, force-drops them, and verifies that tables and databases with expired recycle times are properly cleaned up even when partition erasure takes time