github-actions[bot] commented on code in PR #68283:
URL: https://github.com/apache/doris/pull/68283#discussion_r4058991797
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1358,6 +1362,7 @@ public boolean isProfileSafeStmt() {
private void forwardToMaster() throws Exception {
masterOpExecutor = new MasterOpExecutor(originStmt, context,
redirectStatus, isQuery());
+ throwIfTerminated();
Review Comment:
[P1] Keep forwarded cancellation sticky through master registration. After
this check passes, a concurrent timeout/KILL sees the published executor and
sends `FEOpExecutor.cancel()`, but the master only looks in
`proxyQueryIdToConnCtx`; before the normal forward request reaches
`registerProxyQuery()`, that cancel is acknowledged as a no-op. This thread
then continues to `masterOpExecutor.execute()`, registers the same query id,
and can run the query or mutating command. This is a different window from the
existing cancel-before-publication thread. Please make the cancel/start handoff
sticky across master registration (or otherwise prevent the later start) and
add a latch-based cancel-before-registration test.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/PointQueryExecutor.java:
##########
@@ -254,12 +255,16 @@ void addKeyTuples(
@Override
public void cancel(Status cancelReason) {
- // Do nothing
+ this.cancelReason = cancelReason;
+ isCancel = true;
}
@Override
public RowBatch getNext() throws Exception {
+ if (isCancel) {
Review Comment:
[P1] Fence cancellation while the point lookup is in flight. Once
`getNext()` reads `isCancel == false` here, `cancel(Status)` only flips the
flag; it neither cancels the active future nor prevents the valid empty/row
branches from returning before the later check at line 405 (which is
unreachable for valid responses). `StmtExecutor` can therefore send rows after
TIMEOUT/KILL. This is distinct from the existing pre-lookup thread. Recheck the
retained reason before retries and every successful/empty return, cancel or
fence the active future, and add a barrier-based in-flight cancellation test.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -220,7 +221,7 @@ public class StmtExecutor {
// the idle reaper's bound and the profile's times counted from its own
start.
private volatile TUniqueId deferredQueryId;
private volatile long deferredStartTimeMs = -1;
- private MasterOpExecutor masterOpExecutor = null;
+ private volatile MasterOpExecutor masterOpExecutor = null;
Review Comment:
[P1] Do not share the forwarded result between execution and cancellation.
Publishing this executor lets `execute()` and `cancel()` run concurrently, but
both inherited methods assign their independent RPC replies to the same
unsynchronized `FEOpExecutor.result`. If the statement response is stored first
and the cancel acknowledgement (status 0, empty packet, no result buffers)
lands last, subsequent proxy/result accessors consume the cancel reply and
discard the real rows or error. Keep the statement response exclusively owned
by `execute()` (use a local/dedicated cancel reply for journal waiting) and add
an overlap test where cancel completes last.
--
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]
