924060929 commented on code in PR #68283:
URL: https://github.com/apache/doris/pull/68283#discussion_r4069443635
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/execute/ConnectorRewriteGroupTask.java:
##########
@@ -106,14 +107,17 @@ public Long getId() {
@Override
public void execute() throws JobException {
- if (isCanceled.get()) {
- throw new JobException("Rewrite task has been canceled, task id: "
+ taskId);
- }
if (isFinished.get()) {
return;
}
Review Comment:
Fixed. `ConnectorRewriteDriver.runGroups` now tracks the groups it actually
submitted and drains them — cancel, then a bounded `awaitTerminal` backed by a
new per-task latch — on timeout, on interruption, and when an `addMemoryTask`
call fails partway, before `run()` rolls the shared transaction back or closes
it. Added three `ConnectorRewriteDriverTest` cases (timeout, interruption,
Nth-submission failure) that assert every submitted group is cancelled and
`rollback(txn)` runs with no `commit(txn)`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/AbstractInsertExecutor.java:
##########
@@ -272,6 +272,16 @@ 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);
+ // Publication synchronously replays any cancellation retained
during planning. Fence on that
+ // terminal status before executor-specific setup runs, so a later
setup failure cannot mask the
+ // original timeout/cancel reason.
+ Status execStatus = coordinator.getExecStatus();
Review Comment:
Fixed. `AbstractInsertExecutor.executeSingleInsert` now re-fences
`coordinator.getExecStatus()` after the completion listeners and immediately
before `onComplete()`, so a TIMEOUT/KILL that lands after `execImpl()`'s last
status read cannot be followed by a commit. This is the common completion
boundary, covering row-level UPDATE/DELETE/MERGE. Added
`OlapInsertExecutorTest.testCancellationAfterLastStatusReadFencesBeforeCommit`,
which flips the coordinator status at `beforeComplete` and asserts the
transaction is aborted and the original reason reaches `QueryState`.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/NereidsCoordinator.java:
##########
@@ -197,7 +199,24 @@ public void cancel(Status cancelReason) {
LOG.warn("Cancel execution of query {}, this is a outside invoke,
cancelReason {}",
DebugUtil.printId(queryId), cancelReason);
}
Review Comment:
Fixed. `NereidsCoordinator.cancel()` now starts the queue-token/scan cleanup
and the final `cancelInternal` in a `finally` around `updateStatusIfOk`, so a
partially initialized processor cancel cannot skip them, and
`LoadProcessor.cancel()` guards the latch with `ifPresent()` instead of
`latch.get()`. Added
`PipelineExecutionTaskTest.loadProcessorCancelToleratesTaskPublishedBeforeLatch`
for the publication window and
`NereidsCoordinatorTest.testCancelStillCleansUpWhenStatusPublicationFails`,
which asserts the scan cleanup and final internal cancel still run when status
publication throws.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/Coordinator.java:
##########
@@ -767,6 +767,10 @@ private boolean shouldQueue() {
// A call to Exec() must precede all other member function calls.
@Override
public void exec() throws Exception {
+ Status status = getQueryStatus();
Review Comment:
Fixed in both coordinator paths. `Coordinator.preferTerminalReason(...)`
re-reads the coordinator's first terminal status after a queue wait is
cancelled, so a TIMEOUT/KILL that unblocked `QueueToken.get()` wins over the
token's generic "query is cancelled"; a pure queue timeout keeps its own
message. Wired into legacy `exec()` and Nereids `enqueue()`, with
`testQueueCancellationPrefersRetainedTerminalReason` added to both coordinator
tests.
--
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]
