Conversation
…annel Rebuilding only the MV partitions that a silent base-table change really invalidated needs a per-partition state: which generation of data the partition holds and which generation it must hold, persisted so that it survives a restart. This adds that state and the journal channel that carries it. Nothing decides anything from it yet and nothing but a replay writes it, so the behaviour of every MV is unchanged. Key changes: - Add MTMVPartitionState, a persisted refreshEpoch / latestEpoch pair keyed by MV partition name, and MTMVPartitionState.copyOf for taking a detached snapshot of a state map - Add MTMV.partitionStates with its getter and its replay setter; gsonPostProcess initializes it, so an image written before the field existed and a non-IVM MV both load as an empty map - Carry the state in the ADD_TASK payload under the same condition as ivmInfo, which keeps the journal of a non-IVM MV byte-for-byte what it was, and apply it on replay only when the field is present - Add MTMVAlterOpType.ALTER_PARTITION_STATES and its Alter.processAlterMTMV branch, so a live state change has an op of its own instead of riding on ALTER_IVM_INFO, whose branch only swaps the IvmInfo object Unit Test: - MTMVTest: an image round trip of a state, an image without the field, the getter before any state exists, the detached journal payload, the ADD_TASK carry for an IVM MV and its absence for a non-IVM MV, and both replay directions - AlterMTMVTest: replay of ALTER_PARTITION_STATES through Alter.processAlterMTMV on a real MV
yujun777
requested review from
gavinchou,
luwei16,
morrySnow and
mymeiyi
as code owners
September 18, 2026 09:35
yujun777
marked this pull request as draft
September 18, 2026 09:36
yujun777
marked this pull request as ready for review
September 18, 2026 09:36
yujun777
marked this pull request as draft
September 18, 2026 09:37
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
yujun777
marked this pull request as ready for review
September 18, 2026 09:37
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.
Static review conclusion: request changes for two P2 issues: the new partition-state API exposes mutable owned state after releasing its lock, and the new journal channel is not tested across its actual AlterMTMV.write/read representation.
Critical checkpoint conclusions:
- Goal and proof: the change is focused on adding inert per-partition epoch state plus image, ADD_TASK, and dedicated alter replay channels. The production serialization path is structurally present, but the journal wire/restart contract lacks a round-trip oracle.
- Scope and clarity: the seven changed FE files are cohesive and do not add refresh/invalidation policy. The additive DTO and replay branches are otherwise small and clear.
- Concurrency and locking: task completion snapshots and enqueues under
mvRwLock, the FIFO edit-log queue preserves order, andawait()is correctly outside the lock. No new lock-order or deadlock issue was found. However,getPartitionStates()returns the live map and mutable values after unlocking, bypassing that ownership invariant for the follow-up callers this API is meant to support. - Lifecycle: old images initialize the missing field to empty; old ADD_TASK records with no member preserve current state; an explicit empty map clears it; replay applies detached copies. INSERT OVERWRITE keeps the formal partition name while changing its id, and no current production logic populates or consumes the map. Future sync drop/add logic must own state removal/reinitialization atomically.
- Configuration and initialization: no configuration item, static-initialization dependency, or new thread is introduced.
- Compatibility: the image and ADD_TASK fields are additive Gson members; non-IVM ADD_TASK payloads remain unchanged; the dedicated enum operation is not emitted by live code in this PR. No FE-BE protocol or storage-format change applies.
- Parallel paths and conditions: IVM/non-IVM, image, ADD_TASK success/failure replay, and dedicated alter replay were traced. The IVM-only live carry condition is consistent with this PR's stated scope.
- Tests and results: the added unit tests cover image compatibility, detached copies, absent ADD_TASK state, IVM/non-IVM carry, and in-memory replay, but not the actual alter-journal bytes or the absent-versus-empty distinction after deserialization. No result files changed. Per the runner instruction, this review was static-only: I did not run builds or tests. CheckStyle had passed externally; FE UT and compile were still pending when inspected, so they are not treated as independent validation here.
- Observability: because the state is intentionally inert in this PR, no new logging or metric is required yet.
- Persistence/failover and data correctness: detached copying before asynchronous serialization, fatal edit-log failure behavior, dispatch, and replay ordering are otherwise correct. There is no direct user-data write or transaction-protocol change.
- Performance: full-map copying is linear but currently empty/inert and is necessary for a detached payload; no separate confirmed performance defect was found.
- User focus and completion: no additional user focus was supplied. Two complete review rounds were performed; all normal and risk-focused reviewers returned
NO_NEW_VALUABLE_FINDINGSin the convergence round, and every candidate was accepted, deduplicated, or dismissed with evidence.
| if (partitionStates == null) { | ||
| partitionStates = Maps.newLinkedHashMap(); | ||
| } | ||
| return partitionStates; |
Contributor
There was a problem hiding this comment.
[P2] Keep partition-state ownership inside mvRwLock
This returns the live map after the finally releases mvRwLock, and each value is mutable too. A caller using the pattern in the new tests (getPartitionStates().put(...)) can therefore add/remove entries or change an epoch while addTaskResult() is copying the same LinkedHashMap for the journal, producing a ConcurrentModificationException or a mixed snapshot. If replay replaces the field first, the retained reference instead accepts a silently lost update. Since this API is the persistence foundation for the follow-up invalidation/alignment code, please return a deep detached/unmodifiable snapshot for reads and add lock-owning MTMV mutation methods that mutate and enqueue ALTER_PARTITION_STATES under the same write lock.
| // The MV starts without any state, so only the replayed payload can put it there. | ||
| mtmv.alterPartitionStates(Map.of()); | ||
|
|
||
| Env.getCurrentEnv().getAlterInstance().processAlterMTMV(replayAlter, true); |
Contributor
There was a problem hiding this comment.
[P2] Exercise the journal wire round trip before replay
This calls processAlterMTMV with the same in-memory object created above, and the ADD_TASK tests likewise inspect an object captured by a mocked submitEdit. As a result, removing or mis-serializing the new op/pst member—or collapsing the required absent-versus-empty distinction—would leave every new replay test green even though restart/failover is this PR's main deliverable. Please round-trip AlterMTMV through write/read (or JournalEntity) before replay and cover present nonempty, present empty (clear), and absent old payload (preserve).
Contributor
TPC-H: Total hot run time: 28309 ms |
Contributor
TPC-DS: Total hot run time: 153758 ms |
Contributor
ClickBench: Total hot run time: 23.98 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
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?
Trace issue: #65418
This PR adds no behaviour of its own. It adds the state that the following PRs need, and the channel
that persists it, so that they can be reviewed as logic alone.
An IVM materialized view has to invalidate the MV partitions that a base-table change really affected.
A partition drop / truncate / replace / recover changes the base table through metadata and emits no
row binlog, so the affected MV partitions must be rebuilt; today the only answer the MV has is "rebuild
all of them", which throws away partitions that are still correct.
Deciding per partition needs a per-partition answer to two questions:
That pair is
MTMVPartitionState { refreshEpoch, latestEpoch }, one entry per MV partition, keyed bypartition name.
latestEpochis the requirement,refreshEpochis the reality, and a partition whoserequirement is ahead of its reality is dirty: it holds rows read before a change that left no binlog,
so it can no longer be maintained incrementally and has to be rebuilt. The requirement has to survive a
restart, because an invalidation that only lives in memory is lost the moment the FE restarts, and a
partition that is then refreshed incrementally keeps the stale rows forever with no error anywhere.
So this PR adds
MTMVPartitionState, plus a copy helper for taking a detached snapshot), andrecord, a dedicated alter op with its replay branch, and the replay handling of the task result.
Nothing in the FE decides anything from the state yet, and nothing but a replay ever writes it, so every
MV behaves exactly as before. That is deliberate: it makes this step independently mergeable and
independently testable, which is what the PR that starts using the state needs underneath it.
Scope
MTMVPartitionState, thepartitionStatesfield, the alter record field, the new alter op and its replay branchIvmInfo; the refresh pathThe state is on the MV rather than inside
IvmInfo, and the alter op is its own rather than riding onALTER_IVM_INFO, whose branch only swaps theIvmInfoobject. Both are structural: the same state ismeant to serve a non-IVM MV later, and its journal payload must not be reconstructed as a side effect of
replaying some other op.
Key changes
MTMVPartitionState, a persistedrefreshEpoch/latestEpochpair keyed by MV partition name, andMTMVPartitionState.copyOffor taking a detached snapshot of a state map. A partition gets a new id on every refresh, so the name is the only identity it can have.MTMV.partitionStateswith its getter and its replay setter;gsonPostProcessinitializes it, so an image written before the field existed and a non-IVM MV both load as an empty map.ivmInfo, which keeps the journal of a non-IVM MV byte-for-byte unchanged, and apply it on replay only when the field is present, so an old journal applies nothing rather than clearing the state.MTMVAlterOpType.ALTER_PARTITION_STATESand itsAlter.processAlterMTMVbranch.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
🤖 Generated with Claude Code