github-actions[bot] commented on code in PR #68283:
URL: https://github.com/apache/doris/pull/68283#discussion_r4059188753
##########
fe/fe-core/src/main/java/org/apache/doris/qe/MasterOpExecutor.java:
##########
@@ -59,24 +66,52 @@ public MasterOpExecutor(ConnectContext ctx) {
@Override
public void execute() throws Exception {
- super.execute();
- waitOnReplaying();
+ synchronized (executionLock) {
+ if (cancelReason != null) {
+ throw new UserException("Forwarded statement was terminated
before execution: "
+ + cancelReason.getErrorMsg());
+ }
+ executionStarted = true;
+ }
+ try {
+ super.execute();
+ waitOnReplaying(result);
+ } finally {
+ synchronized (executionLock) {
+ executionFinished = true;
+ }
+ }
}
@Override
public void cancel() throws Exception {
- super.cancel();
- waitOnReplaying();
+ cancel(Status.CANCELLED);
+ }
+
+ public void cancel(Status reason) throws Exception {
+ synchronized (executionLock) {
+ if (cancelReason == null) {
+ cancelReason = reason;
+ }
+ if (!executionStarted || executionFinished || cancelForwarded) {
+ return;
+ }
+ cancelForwarded = true;
Review Comment:
[P1] Leave failed cancel delivery retryable. `cancelForwarded` is set before
`forwardCancel()` succeeds, so a client-pool or transport failure leaves it
true; every later KILL/timeout then returns at line 96 without sending anything
while the statement RPC may still complete. Treat the flag as
in-flight/delivered and restore a retryable state when the cancel send fails
(duplicate cancel is safe), with a test where the first cancel send throws and
the second succeeds.
##########
fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java:
##########
@@ -397,8 +399,45 @@ public class FrontendServiceImpl implements
FrontendService.Iface {
private ConcurrentHashMap<Long, AtomicInteger>
multiTableFragmentInstanceIdIndexMap =
new ConcurrentHashMap<>(64);
- private final Map<TUniqueId, ConnectContext> proxyQueryIdToConnCtx =
- new ConcurrentHashMap<>(64);
+ private final ProxyQueryRegistry proxyQueryRegistry = new
ProxyQueryRegistry();
+
+ static final class ProxyQueryRegistry {
+ private final Object lock = new Object();
+ private final Map<TUniqueId, ConnectContext> contexts = new
HashMap<>();
+ private final Cache<TUniqueId, Status> pendingCancels =
CacheBuilder.newBuilder()
+ .maximumSize(1_000_000)
+ .expireAfterWrite(30, TimeUnit.MINUTES)
+ .build();
+
+ Runnable register(TUniqueId queryId, ConnectContext context) {
+ Status pendingCancel;
+ synchronized (lock) {
+ contexts.put(queryId, context);
Review Comment:
[P1] Keep cancellation terminal across same-ID forward retries.
`FEOpExecutor.forward()` can retry a query with the same request/query ID after
a non-timeout transport failure. If attempt 1 is registered when cancellation
arrives, `cancel()` stops only that context and stores no tombstone; after
cleanup, the retry registers here with no terminal state and can execute/return
rows. Overlapping attempts are also collapsed to one map entry. Retain
cancellation for every registration in the query-ID retry lifetime (and track
all live contexts, or fence the origin retry), with a
cancel-C1-then-register-C2 test.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1621,29 +1651,34 @@ public void executeAndSendResult(boolean
isOutfileQuery, boolean isSendFields,
// Where the result goes is the protocol's decision, made now, before
the coordinator is
// built: relayed by this frontend through the sender, or left on the
backends for the
// client to pull (context.isReturnResultFromLocal() is false then).
+ throwIfTerminated();
Review Comment:
[P1] Recheck termination after coordinator publication and before OUTFILE
cleanup. A TIMEOUT/KILL can pass this gate while the coordinator is being
constructed; `setCoord()` then replays the terminal reason, but control still
reaches `deleteExistingOutfileFilesInFe()` before `coordBase.exec()` rejects
the cancelled coordinator. With `delete_existing_files`, that recursively
removes the existing remote output and the cancelled query writes no
replacement. Add the post-publication gate before deletion and a
cancel-during-construction OUTFILE test.
##########
fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java:
##########
@@ -1248,10 +1287,8 @@ private TMasterOpResult
handleForwardCancel(TMasterOpRequest params) throws TExc
if (!params.isSetQueryId()) {
throw new TException("a query id is needed to cancel a query");
}
- ConnectContext context =
proxyQueryIdToConnCtx.get(params.getQueryId());
- if (context != null) {
- context.cancelQuery(new Status(TStatusCode.CANCELLED, "cancel
query by forward request."));
- }
+ proxyQueryRegistry.cancel(params.getQueryId(),
Review Comment:
[P2] Preserve the first terminal reason across forwarded cancellation. The
origin retains `TIMEOUT` and passes it to `MasterOpExecutor.cancel(reason)`,
but the request carries only a boolean/query ID and this handler always
rebuilds `CANCELLED`, so the master/BE, client message, and follower audit lose
the timeout cause. Carry optional code/message fields with a rolling-version
fallback, and recheck the retained reason after `masterOpExecutor.execute()`
before hooks/result selection. Add an in-flight TIMEOUT propagation test.
--
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]
