Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
1deb990 to
2264160
Compare
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found two P2 regressions and one P3 reverse-index retention issue. The O(k) direction is sound, but the current implementation does not yet preserve complete cleanup across legacy migration and follower replay.
Critical checkpoints
- Goal and scope: The patch is focused and reduces ordinary unregister/stale-prune work from global forward-map scans to work proportional to the MTMV's prior dependencies. The modern no-change compatibility fast path is correctly signaled.
- Concurrency: Ordinary refresh completion and unregister share the MTMV reentrant write lock. Follower compatibility registration is the exception: post-processing overlaps the live replayer, and the unlocked re-register can publish concurrently with replay, leaving a forward edge outside the surviving reverse snapshot.
- Lifecycle and compatibility: Image registration precedes ID-to-name compatibility. Because
BaseTableInfo.compatiblemutates fields used byhashCode, targeted lookup cannot reach the legacy forward-map bucket, so the new cleanup retains dependency descriptors that the parent full scan removed. No wire-format or FE-BE compatibility change was introduced. - Parallel paths and conditions: All three forward/reverse categories were traced. Empty category transitions prune forward membership but retain the prior reverse snapshot; null relations and stable nonempty replacements otherwise behave consistently.
- Persistence and data correctness: Reverse indexes are in-memory only. Image load, create replay, ADD_TASK replay, checkpoint isolation, and promotion after replayer shutdown were traced; no separate journal, transaction, visible-version, or data-write issue was found.
- Tests: Static review only; the runner contract forbids local builds/tests. The changed tests cover stable-key replacement/idempotence and modern false compatibility, but the named unregister test actually exercises empty stale-pruning. Legacy true conversion, public reverse-backed unregister with production equality, and the follower replay interleaving remain uncovered. GitHub currently reports COMPILE, CheckStyle, and FE UT passing; the Doris Performance status is failing with only an opaque TeamCity result.
- Configuration, protocol, observability: No new configuration, FE-BE variable, storage format, metric, or additional observability obligation applies.
User focus
No additional user-provided focus was specified; the complete PR was reviewed.
| removeMTMV(mtmvInfo); | ||
| return; | ||
| } | ||
| Set<BaseTableInfo> staleTables = mtmvToBaseTables.get(mtmvInfo); |
There was a problem hiding this comment.
[P2] Serialize compatibility registration with replay refreshes
On a follower, transferToNonMaster leaves the replayer running while postProcessAfterMetadataReplayed calls MTMV.compatible. That method releases the MV write lock after unregister and performs registerMTMV unlocked. If compatibility captures relation R0 while an ADD_TASK replay publishes R1, both refreshes can read the same prior reverse snapshot, add different forward edges, and leave only the last reverse value; the new reverse-only unregister then cannot remove the uncovered edge. Please hold the MV write lock across the compatibility unregister/register replacement or make the per-MV forward/reverse replacement atomic, and cover this interleaving with latches.
| @@ -240,7 +249,9 @@ private void addMTMVTables(Set<BaseTableInfo> baseTables, BaseTableInfo mtmvInfo | |||
| if (CollectionUtils.isEmpty(baseTables)) { | |||
There was a problem hiding this comment.
[P3] Clear the reverse entry when this category becomes empty
When an MV refresh changes this category from nonempty to empty, this return leaves mtmvToBaseTables pointing at the old bases. removeMTMVFromStaleRelations removes the forward memberships, but nothing removes the reverse snapshot, so each affected long-lived MV retains its last nonempty dependency set until another nonempty refresh or unregister. The view and one-level adders have the same branch. Please remove or replace the reverse entry on the empty transition and cover the actual reverse-backed unregister path; the new empty-relation test only exercises stale pruning.
| for (Set<BaseTableInfo> sets : tableMTMVsOneLevelAndFromView.values()) { | ||
| sets.remove(mtmvInfo); | ||
| for (BaseTableInfo base : bases) { | ||
| Set<BaseTableInfo> mtmvs = forwardMap.get(base); |
There was a problem hiding this comment.
[P2] Do not clean legacy entries through their mutated hash
Image loading registers legacy ID-only BaseTableInfo objects before compatible() fills ctlName/dbName/tableName, and those same mutable fields define hashCode(). The reverse snapshot keeps the same objects, so forwardMap.get(base) probes their new hash and cannot reach the old ConcurrentHashMap bucket; removing the reverse entry then makes those old value sets permanently uncleanable, while re-registration adds a second name-hashed entry. The previous full value-set scan at least removed the MTMV values. Please rebuild before mutation, use stable immutable keys, or retain a compatibility-only cleanup fallback, and add an ID-only upgrade test.
2 participants
What problem does this PR solve?
We found that when the number of mtmv in the cluster reaches a large number (e.g. 80,000+) , the restart of fe becomes very slow.
The jstack of a fe that is currently being restarting is captured as follows:
And there are amount of FE logs like this:
or
Each invocation of the
registerMTMV()function takes about 20ms (about 5 base tables per mtmv, most spent in removeMTMV()), theEnv.loadDb()andENV.postProcessAfterMetadataReplayed()methods of the checkpoint thread invoke theregisterMTMV()function once for each mv，and thetransferToMaster()will also invoke theregisterMTMV()function once for each mv.Therefore, the total time of transfering to master + restarting fe with 80000+ mtmv is 4800+s（about 1.5h）, this will have a serious impact on the checkpoint/transfer-master/restart duration of fe.
This commit:
1、Skip doing
registerMTMV()in MTMV compatible when there is nothing was actually migrated.2、Add reverse index reduce the time complex of
registerMTMV()/unregisterMTMV()from O(N^2) to O(k) in the mtmv's own base countProblem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)