github-actions[bot] commented on code in PR #68283:
URL: https://github.com/apache/doris/pull/68283#discussion_r4058758922
##########
fe/fe-core/src/main/java/org/apache/doris/qe/Coordinator.java:
##########
@@ -767,6 +767,9 @@ private boolean shouldQueue() {
// A call to Exec() must precede all other member function calls.
@Override
public void exec() throws Exception {
+ if (isQueryTerminated()) {
Review Comment:
[P1] Make queue-token publication cancellation-aware. `isQueryTerminated()`
is checked before queue admission, but the status check is not serialized with
publishing the token. If TIMEOUT arrives after this check and before
`queueToken` is assigned, `Coordinator.cancel()` sees null; `shouldQueue()`
only filters CANCELLED, so execution can block in `QueueToken.get()` on a
future that can no longer be cancelled until queue admission/timeout.
Re-check/cancel immediately after publishing the token, and apply the same
hand-off in `NereidsCoordinator.enqueue`.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1419,6 +1420,7 @@ public void clearCancelDelegate() {
}
Review Comment:
[P1] Replay pending cancellation when forwarding to the master FE.
`pendingCancelReason` is replayed only by `setCoord`, but `forwardToMaster()`
publishes a `MasterOpExecutor` directly and immediately calls `execute()`. If
KILL/TIMEOUT arrives during analysis before that assignment, `cancel()` records
the reason while both `masterOpExecutor` and `coord` are null, then returns;
the later forward RPC executes remotely despite the terminal statement. Make
forward publication cancellation-aware and cover cancel-before-forward.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/Coordinator.java:
##########
@@ -919,6 +922,9 @@ protected void execInternal() throws Exception {
protected void sendPipelineCtx() throws Exception {
lock();
try {
Review Comment:
[P2] Avoid registering instances after terminal cancellation.
`registerInstances()` still runs during `execInternal()` before this new
dispatch fence, so a cancellation during fragment assignment increments
`queryToInstancesNum`, per-user `max_query_instances` accounting, and the begin
metric; it can also throw `reach max_query_instances`, masking the recorded
TIMEOUT/CANCELLED status even though dispatch is rejected here. Make
registration part of the cancellation-aware admission (including the analogous
Nereids check/register pair) and add a test that registration is skipped.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1419,6 +1420,7 @@ public void clearCancelDelegate() {
}
Review Comment:
[P1] Close the late nested-delegate hand-off. In
`RefreshMTMVCommand.dryRunRefresh`, internal context and delta-plan
construction precede `setCancelDelegate`; a timeout/KILL in that window is
stored here while no delegate exists, and the setter only assigns the callback.
The internal executor then starts without receiving the pending reason. Replay
the pending status atomically when installing the delegate (or gate nested
execution) and add cancel-before-registration coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1419,6 +1420,7 @@ public void clearCancelDelegate() {
}
public void cancel(Status cancelReason, boolean needWaitCancelComplete) {
+ pendingCancelReason.compareAndSet(null, cancelReason);
Consumer<Status> delegate = cancelDelegate;
if (delegate != null) {
delegate.accept(cancelReason);
Review Comment:
[P1] Apply the pending terminal state to non-Coordinator execution paths.
FE-only results and cache replay return before `setCoord`, while the prepared
short-circuit branch keeps a local `PointQueryExecutor` whose `cancel()` is a
no-op and whose `getNext()` still performs the tablet lookup. A TIMEOUT/KILL
during planning can therefore still return rows or do BE work after
cancellation. Add a statement-level gate or cancellation-aware interface before
these branches, with negative 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]
