github-actions[bot] commented on code in PR #68283:
URL: https://github.com/apache/doris/pull/68283#discussion_r4059387917
##########
fe/fe-core/src/main/java/org/apache/doris/qe/NereidsCoordinator.java:
##########
@@ -154,9 +154,15 @@ private void syncLocalShufflePlannerOption() {
@Override
public void exec() throws Exception {
+ if (isQueryTerminated()) {
+ throw new UserException("Query was terminated before execution");
Review Comment:
[P2] Keep this new `UserException` compatible with dictionary INSERT failure
handling. A pending cancellation is replayed before `exec`, this fence throws
plain `UserException`, and `AbstractInsertExecutor.executeSingleInsert` passes
it to `DictionaryInsertExecutor.onFail`, which unconditionally casts every
`Throwable` to `DdlException`. Because `DdlException` is a subclass of
`UserException`, that cast throws `ClassCastException` and masks the
cancellation instead of setting QueryState. Handle arbitrary execution failures
in `DictionaryInsertExecutor` (preserving a MySQL code only when available) and
add a pending TIMEOUT/CANCELLED dictionary-insert test.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1419,6 +1422,7 @@ public void clearCancelDelegate() {
}
Review Comment:
[P2] Forward the retained first reason during publication. The CAS records
the first terminal status, but later calls still send their own argument to a
published coordinator. For example, TIMEOUT can win this CAS while `coord` is
null; `setCoord` can publish and pause; then a later KILL loses the CAS but
calls `coord.cancel(CANCELLED)` before `setCoord` replays TIMEOUT. Both
coordinator implementations keep the first status they receive, so CANCELLED
permanently replaces the earlier TIMEOUT. This is local to the new handoff, not
the removed cross-FE path. Route the retained `AtomicReference` value to the
coordinator, or serialize publication/cancellation, and add a barrier test with
two different reasons crossing `setCoord`.
##########
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()) {
+ throw new UserException("Query was terminated before execution");
Review Comment:
[P2] Surface the retained terminal reason from this fence. `setCoord` has
already installed the original TIMEOUT/CANCELLED `Status`, but this fresh
`UserException` (and the parallel legacy dispatch, Nereids, and pipeline-task
fences) replaces it with generic text. INSERT/DML catches that exception and
`onFail` builds QueryState/load error text from `t.getMessage()`, while normal
query handling also reports it to the client/audit, so a timeout becomes only
`Query was terminated ...`. This is separate from the removed cross-FE
transport code. Fail with the stored status/message, preserve its
timeout/cancel classification where supported, and assert it through
`StmtExecutor`/QueryState for both coordinator paths.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1449,6 +1453,14 @@ public void cancel(Status cancelReason, boolean
needWaitCancelComplete) {
Review Comment:
[P1] Make coordinator publication mandatory for every statement-owned
writer. This setter is currently opt-in: `InsertIntoTVFCommand`
creates/registers a local coordinator and calls `exec()` without `setCoord`,
and `RewriteTableCommand`/`ConnectorRewriteGroupTask` execute their
`AbstractInsertExecutor` coordinators without publishing them either. A
timeout/KILL during those planning windows is retained here while `coord` stays
null, then the external write still dispatches (TVF can also delete existing
files first). These are coordinator-backed paths, so they are distinct from the
existing FE-only/point-query thread. Publish them before destructive
setup/dispatch, or centralize the handoff in the common execution boundary, and
add cancel-during-planning tests for TVF and connector rewrite.
--
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]
