github-actions[bot] commented on code in PR #68283:
URL: https://github.com/apache/doris/pull/68283#discussion_r4069347619
##########
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:
[P2] Preserve the retained reason when queue waiting is cancelled. This
entry check only covers cancellation before queueing. If TIMEOUT/KILL lands
after the token is published while QueueToken.get() is waiting, cancel() stores
the detailed status and cancels the token, but QueueToken.get() throws the
generic "query is cancelled"; insert/DML failure handling then reports that
text before another status fence can translate it. Re-read the coordinator
status after queue cancellation (in both coordinator paths), or carry the
Status through the token, and add blocked-token TIMEOUT/CANCELLED tests.
##########
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:
[P1] Fence cancellation again before committing row-level DML. execImpl
performs its last status read before returning, but UPDATE/DELETE/MERGE have
neither InsertIntoTableCommand's cancellation listener nor another
coordinator-status check before onComplete. If TIMEOUT/KILL completes after
that read and before this completion phase, QueryState remains OK and
BaseExternalTableInsertExecutor commits the connector transaction even though
cancellation already won. Add a common cancellation-aware pre-commit handoff
(not just an unsynchronized message check) and a latch-controlled row-level DML
rollback/no-commit test.
##########
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:
[P1] Put the first internal cancel inside this cleanup guard.
setPipelineExecutionTask publishes executionTask before
LoadProcessor.afterSetPipelineExecutionTask initializes its latch; a
TIMEOUT/KILL in that window reaches updateStatusIfOk -> LoadProcessor.cancel,
sees the task, and throws from latch.get(). Because that happens before this
try, token/scan cleanup and the final resend are skipped, and a broker-load
cancellation caller can leave its state transition unfinished. Publish the
task/latches atomically or start the non-throwing cleanup scope before
updateStatusIfOk, with a latch-controlled crossing test.
##########
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:
[P1] Drain live rewrite groups before rolling back their shared transaction.
This task now cooperates with queued/running cancellation, but the driver
invokes that path only after a group reports failure. If the owner times out,
is interrupted, or a later addMemoryTask call fails, runGroups returns without
cancelling already-submitted tasks and run() immediately rolls back/closes the
transaction they still bind and report into. Cancel every submitted/registered
task and wait for terminal callbacks before rollback, with timeout/interruption
and Nth-submission crossing 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]
