github-actions[bot] commented on code in PR #68038:
URL: https://github.com/apache/doris/pull/68038#discussion_r4022375400
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVRelationManager.java:
##########
@@ -218,12 +224,15 @@ public void refreshMTMVCache(MTMVRelation relation,
BaseTableInfo mtmvInfo) {
removeMTMV(mtmvInfo);
return;
}
+ Set<BaseTableInfo> staleTables = mtmvToBaseTables.get(mtmvInfo);
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVRelationManager.java:
##########
@@ -258,29 +271,48 @@ private void
addMTMVTablesOneLevelAndFromView(Set<BaseTableInfo> baseTables, Bas
if (CollectionUtils.isEmpty(baseTables)) {
return;
}
- for (BaseTableInfo baseTableInfo : baseTables) {
+ Set<BaseTableInfo> snapshot = ImmutableSet.copyOf(baseTables);
+ mtmvToBaseTablesOneLevelAndFromView.put(mtmvInfo, snapshot);
+ for (BaseTableInfo baseTableInfo : snapshot) {
getOrCreateMTMVsOneLevelAndFromView(baseTableInfo).add(mtmvInfo);
}
}
private void removeMTMV(BaseTableInfo mtmvInfo) {
- for (Set<BaseTableInfo> sets : tableMTMVs.values()) {
- sets.remove(mtmvInfo);
- }
- for (Set<BaseTableInfo> sets : viewMTMVs.values()) {
- sets.remove(mtmvInfo);
+ removeFromForwardMap(tableMTMVs, mtmvToBaseTables, mtmvInfo);
+ removeFromForwardMap(viewMTMVs, mtmvToBaseViews, mtmvInfo);
+ removeFromForwardMap(tableMTMVsOneLevelAndFromView,
mtmvToBaseTablesOneLevelAndFromView, mtmvInfo);
+ }
+
+ private void removeFromForwardMap(Map<BaseTableInfo, Set<BaseTableInfo>>
forwardMap,
+ Map<BaseTableInfo, Set<BaseTableInfo>> reverseIndex, BaseTableInfo
mtmvInfo) {
+ Set<BaseTableInfo> bases = reverseIndex.remove(mtmvInfo);
+ if (bases == null) {
+ return;
}
- for (Set<BaseTableInfo> sets : tableMTMVsOneLevelAndFromView.values())
{
- sets.remove(mtmvInfo);
+ for (BaseTableInfo base : bases) {
+ Set<BaseTableInfo> mtmvs = forwardMap.get(base);
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVRelationManager.java:
##########
@@ -240,7 +249,9 @@ private void addMTMVTables(Set<BaseTableInfo> baseTables,
BaseTableInfo mtmvInfo
if (CollectionUtils.isEmpty(baseTables)) {
Review 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.
--
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]
