yujun777 commented on code in PR #68390:
URL: https://github.com/apache/doris/pull/68390#discussion_r4118233354
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -862,41 +1070,53 @@ &&
hasUnusableIvmStreamForPartitions(partitionPlan.context, needRefreshPartition
if (refreshMode == MTMVTaskRefreshMode.NOT_REFRESH) {
return true;
}
- writeIvmBaselineBarrier(RefreshMode.PARTITIONS);
- executePartitionBasedRefresh(partitionPlan.context,
RefreshMode.PARTITIONS, ctx);
+ executePartitionBasedRefresh(partitionPlan.context,
RefreshMode.PARTITIONS, ctx,
+ partitionPlan.partitions);
return true;
}
Review Comment:
Confirmed, and it is worse than "selected" reset scans -- a partition
refresh of an entirely *clean* partition moves those offsets. `d69bd30ac9f`
restores the durable requirement, and I measured the loss rather than reasoning
about it.
**The mechanism, with the anchors.** `InsertOverwriteTableCommand` commits
the rows into temporary partitions (`insertIntoPartitions`) and publishes them
with a swap afterwards (`replacePartition`); what the read consumed is
committed with the first half, not the second --
`StreamConsumptionInfoExtractor` at the insert's `beforeComplete` hangs the
`TableStreamUpdateInfo`s on the transaction, and
`AbstractInsertExecutor#requiresTransaction` says so outright ("A Table Stream
offset update must be committed even when optimization proves that the target
receives no rows"). Three ways in: an exception in or after the swap (`catch`
-> `taskFail`), a master switch (`InsertOverwriteManager#allTaskFail`, "try
drop all temp partitions when transferToMaster"), and a cancellation -- which
hits the explicit early return between the two halves.
The scope is every partition refresh of an IVM MV, not the reset-id subset:
`MTMVTask#collectPctResetPartitionIds` names *every* base partition the
refreshed MV partitions map to, without asking whether their MV partitions are
dirty, and `IvmFullRefreshMTMV` then rewrites those scans to
`StreamReadMode.RESET`. So a plain "the base table changed, refresh this
partition" refresh moves the offset, and its MV partition is clean, which is
exactly the state where a later incremental refresh reads past the change and
reports success.
**The measurement.** The new suite injects a failure at that boundary and
asserts the recovery. With the raise in `executePartitionBasedRefresh` removed
(and nothing else changed), it fails on the data, not on the route: the failed
refresh's rows are gone with the temporary partitions, the live partition keeps
the rows it had, and the following strict `INCREMENTAL` reports SUCCESS over a
row that never comes back. With it, that refresh reports the partition in
`IvmRebuiltPartitions` and the row lands. Both runs are on the same build, one
line apart.
**The fix.** `MTMV#raiseRebuildRequirement(Set)` raises `latestEpoch` for
the parts of the scope that do not name a requirement yet and reports what each
of those now names -- read and raised in one lock acquisition, because a mark
landing in between would be a requirement raised above the one returned, and
the caller would then record it as met. Journaled as the whole state map like
every record on that channel, and not journaled at all when there was nothing
to raise. `executePartitionBasedRefresh` calls it first, before it reads
anything, and clamps its recorded epochs to what it raised -- that half is not
optional: left at what an earlier attempt planned, the ceiling sits below the
requirement just raised, the partition stays dirty after the replacement that
met it, and every refresh from then on rebuilds it again. The callers whose
scope is already marked raise nothing, so the whole-MV attempt (which marks
every partition before it reconciles the streams) and the incremental
attempt (which rebuilds the partitions an invalidation marked) are unchanged.
`markPartitionsForRebuild` keeps raising unconditionally -- an invalidation has
to outrank a refresh already running.
**The test.**
`InsertOverwriteTableCommand.failBetweenTheTwoHalvesOfAnOverwrite` sits between
`insertIntoPartitions` and `replacePartition`, and is scoped by the MV name it
carries as its own `mv_name` parameter, so enabling it cannot disturb an
overwrite that is not the one under test.
`regression-test/suites/mtmv_p0/ivm/test_ivm_overwrite_failure_between_the_halves.groovy`
runs the failed refresh, pins the task and the rows that were not published,
and then the recovery. Unit tests cover both halves of the epoch change
(`MTMVTest`, `MTMVTaskTest`; 116 tests, checkstyle clean), and the neighbours
whose assertions read the routing (`test_ivm_partitions_after_failed_complete`,
`test_ivm_partition_epoch_rebuild`,
`test_ivm_strict_incremental_rebuilds_invalidated_partitions`) are green.
**Scope, plainly.** The window is not IVM's: any `INSERT OVERWRITE ...
SELECT ... FROM stream(...)` has it, and `INSERT INTO` does not, because its
rows and its offsets commit together -- which is precisely what the two halves
of an overwrite split. The general fix belongs to the overwrite layer, whose
recovery drops the temporary partitions today instead of completing the swap
when the insert committed; that is written up in
`doc/ivm/design/67-overwrite-stream-offset-atomicity.md` (with the alternatives
and what is unverified). This commit restores the MV-side guarantee only. And
one path here is deliberately **not** covered: a cancellation, because that
early return lets the statement succeed, so the batch is recorded as done and
the requirement this commit raises is cleared by its own write-back. That is
the same overwrite-layer decision -- make the cancellation visible, or finish
the swap -- and it needs no further change on the MV side once it is made.
--
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]
