924060929 commented on code in PR #68283:
URL: https://github.com/apache/doris/pull/68283#discussion_r4067982534
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/AbstractInsertExecutor.java:
##########
@@ -272,6 +272,9 @@ private void checkStrictModeAndFilterRatio() throws
Exception {
*/
public void executeSingleInsert(StmtExecutor executor) throws Exception {
try {
+ // Every statement-owned insert coordinator is published at the
common execution boundary.
+ // Cancellation retained during planning is replayed before any
executor-specific setup or dispatch.
+ executor.setCoord(coordinator);
Review Comment:
Fixed. `ConnectorRewriteGroupTask.executeGroup` now converts the
`QueryState.ERR` that `executeSingleInsert` leaves behind (a retained
coordinator cancellation returns normally) into a failure, so a
cancelled/failed group calls `onTaskFailed` instead of `onTaskCompleted`.
`execute()` reports every failure path to the collector, so the driver sees the
first error, skips registering/committing the partial rewrite, and rolls the
shared transaction back. Added
`ConnectorRewriteDriverTest.groupFailureRollsBackTheSharedTransactionWithoutCommit`,
which runs two groups, fails one, and asserts `rollback(txn)` with no
`commit(txn)`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/AbstractInsertExecutor.java:
##########
@@ -272,6 +272,9 @@ private void checkStrictModeAndFilterRatio() throws
Exception {
*/
public void executeSingleInsert(StmtExecutor executor) throws Exception {
try {
+ // Every statement-owned insert coordinator is published at the
common execution boundary.
+ // Cancellation retained during planning is replayed before any
executor-specific setup or dispatch.
Review Comment:
Fixed. `stmtExecutor` is now `volatile`; `executeGroup` publishes it before
any planning and re-checks `isCanceled`, and `cancel()` sets `isCanceled`
before reading the field, so for any interleaving at least one side observes
the other. A cancellation that arrives before the scheduler runs the task now
goes through the same `onTaskFailed` path instead of throwing before the
callback scope and leaving the collector to wait for the full insert timeout.
Covered by the new `ConnectorRewriteGroupTaskTest` (queued cancellation,
cancel-after-publication, cancel-after-finish).
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1449,6 +1454,14 @@ public void cancel(Status cancelReason, boolean
needWaitCancelComplete) {
}
}
Review Comment:
Fixed at the source. Scan cleanup in `Coordinator.cancel()` and
`NereidsCoordinator.cancel()` is now exhaustive and non-throwing: each
`scanNode.stop()` is guarded so one throwing scan cannot skip the remaining
scans or escape to the publication handoff. That handoff previously ran before
`coordBase` was assigned and before the close `try/finally`, so an escaping
exception skipped `coordinator.close()` and masked the retained reason. Since
`SplitAssignment.stop()` is idempotent, a scan whose first `stop()` threw still
removes its registered sources on the close-time retry. The two cleanup tests
now use a throwing scan plus a trailing scan and assert the trailing scan is
still stopped and nothing escapes.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/AbstractInsertExecutor.java:
##########
@@ -272,6 +272,9 @@ private void checkStrictModeAndFilterRatio() throws
Exception {
*/
public void executeSingleInsert(StmtExecutor executor) throws Exception {
try {
+ // Every statement-owned insert coordinator is published at the
common execution boundary.
+ // Cancellation retained during planning is replayed before any
executor-specific setup or dispatch.
+ executor.setCoord(coordinator);
// Pre-execution work may register external resources, so it must
share the transaction cleanup scope.
beforeExec();
Review Comment:
Fixed. `AbstractInsertExecutor.executeSingleInsert` reads
`coordinator.getExecStatus()` immediately after
`executor.setCoord(coordinator)` and throws that retained reason before
`beforeExec()`, so executor-specific setup never runs after a planning-time
TIMEOUT/CANCELLED and `onFail` cannot surface a later setup error instead of
the original reason. Added
`OlapInsertExecutorTest.testPendingCoordinatorTimeoutFencesBeforeExecSetup`,
which asserts `beforeExec` did not run, the coordinator was not executed, and
the original reason is what reaches `QueryState`.
--
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]
