924060929 commented on code in PR #68283:
URL: https://github.com/apache/doris/pull/68283#discussion_r4058908439
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1419,6 +1420,7 @@ public void clearCancelDelegate() {
}
Review Comment:
Fixed in b2a229d0ffd. setCancelDelegate() now performs the late-publication
hand-off by replaying the retained first terminal reason immediately after
publishing the delegate. The crossing race is covered by the volatile delegate
plus atomic pending status: either cancel sees the delegate or the setter sees
the pending status. Tests cover both direct forwarding and cancellation before
delegate publication.
##########
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:
Fixed in b2a229d0ffd. The terminal-status check and QeProcessor instance
registration now share the existing coordinator lock in both implementations,
so registration cannot occur after cancellation has published a terminal
status. Parameterized CANCELLED/TIMEOUT tests override registerInstances() and
verify it is not called.
##########
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:
Fixed in b2a229d0ffd. Statement-level terminal gates now cover post-planning
execution, FE-only results, cache replay, internal-query results, and master
forwarding. PointQueryExecutor is installed as a cancellation delegate and
rejects getNext() before tablet lookup. Tests cover the generic non-coordinator
gate and cancellation before point-query lookup.
##########
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:
Fixed in b2a229d0ffd. Queue/token publication now happens under the existing
coordinator status lock in both coordinator implementations. The publication
path immediately cancels the token and throws with the original terminal reason
if cancellation won the race; otherwise a later cancel observes the published
token. The CANCELLED/TIMEOUT parameterized tests verify that the token is
cancelled and QueueToken.get() is never reached.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1419,6 +1420,7 @@ public void clearCancelDelegate() {
}
Review Comment:
Fixed in b2a229d0ffd. MasterOpExecutor is now safely published before a
statement-level terminal gate. A cancellation before publication is retained
and prevents execute(); a cancellation after publication observes the volatile
executor and cancels it. The cancel-before-forward test verifies that the
master RPC executor is never executed.
--
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]
