github-actions[bot] commented on code in PR #68283:
URL: https://github.com/apache/doris/pull/68283#discussion_r4061426895
##########
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:
[P1] Propagate a failed rewrite group to the shared transaction owner. This
boundary now lets a retained cancellation make coordinator execution fail, but
`executeSingleInsert` catches every non-retry `Throwable`, calls `onFail`, and
returns normally. `ConnectorRewriteGroupTask` therefore reports
`onTaskCompleted`; the driver registers the union of all groups' source paths
and commits the shared transaction. With groups A/B, if B is cancelled before
producing commit data, Iceberg `commitRewriteTxn` can delete A+B originals
while adding only A's replacement. Return an explicit unsuccessful result or
have the group task convert `QueryState.ERR` into an exception before
`onTaskCompleted`, and cover a two-group failure with rollback/no commit.
##########
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:
[P2] Fence the retained terminal status before executor-specific setup.
`setCoord` can successfully replay a planning-time TIMEOUT/CANCELLED and
return, but `beforeExec` still runs before the first coordinator status check.
For a group-commit insert, a concurrent schema-change block then throws from
`beforeExec` and `onFail` exposes that newer error instead of the retained
terminal reason. This differs from the existing fence-message thread because
the coordinator fence is never reached. Check/throw the stored status
immediately after publication (before `beforeExec`), and add a pending-timeout
group-commit test that asserts no setup runs and the original reason reaches
`QueryState`.
##########
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:
[P1] Close the earlier rewrite task-to-executor cancellation handoff. A
group checks `isCanceled` only at execute entry, publishes a plain non-volatile
`stmtExecutor` much later, and never atomically rechecks/registers it. If a
sibling fails around that handoff, `cancel()` can see null/stale state and the
fresh executor has no pending reason for this `setCoord` call, so the cancelled
group still plans and dispatches against a transaction already destined for
rollback. If cancellation wins before worker start, `execute` throws before its
callback scope and the collector instead waits for the full insert timeout.
This is distinct from the existing coordinator-publication thread because the
`StmtExecutor` itself does not exist yet. Make task cancellation, executor
publication, and terminal callback one atomic handoff, with crossing tests for
queued and running tasks.
##########
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:
[P2] Keep fallible cleanup from escaping this publication handoff. A
retained timeout reaches `coordinator.cancel` here, and
`FileQueryScanNode.stop` calls `SplitAssignment.stop` before removing its
registered sources; `stop` throws when asynchronous split scheduling recorded
an exception. Ordinary `executeAndSendResult` calls `setCoord` before assigning
`coordBase` and before its close `try/finally`, so this exception skips
`coordinator.close`, the throwing scan's source removal, and every later scan
stop (and masks the retained terminal reason). This is distinct from the prior
status-before-cleanup thread: status/backend cancellation are now published,
but the new synchronous caller still loses owner cleanup. Make cancellation
cleanup exhaustive/non-throwing here or publish inside an unconditional close
scope, and test pending cancellation with a throwing split assignment.
--
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]
