github-actions[bot] commented on code in PR #66083:
URL: https://github.com/apache/doris/pull/66083#discussion_r3656271008
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertIntoTableCommand.java:
##########
@@ -309,6 +310,8 @@ public void beforeComplete(AbstractInsertExecutor
insertExecutor, StmtExecutor e
// lock after plan and check does table's schema changed to ensure
we lock table order by id.
Review Comment:
[P1] Re-resolve the target after the auto-start wait
`newestTargetTableIf` is captured before a wait that can last roughly five
minutes and intentionally holds no metadata lock. If the table is dropped and
recreated under the same qualified name during that wait, both `targetTableIf`
and this variable still reference the detached old object, so the later ID and
schema comparisons pass old-vs-old and sink finalization starts with the
removed table ID instead of retrying against the replacement. Perform the wait
before this final lookup, or re-fetch by name after it and lock that newly
resolved object; a latch-based drop/recreate test would cover the ordering.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertIntoTableCommand.java:
##########
@@ -309,6 +310,8 @@ public void beforeComplete(AbstractInsertExecutor
insertExecutor, StmtExecutor e
// lock after plan and check does table's schema changed to ensure
we lock table order by id.
TableIf newestTargetTableIf = getTargetTableIf(ctx,
qualifiedTargetTableName);
Review Comment:
[P1] Clean up the registered load job when auto-start fails
For a non-inline `INSERT ... SELECT` with legacy distribution, the
`OlapInsertExecutor` has already registered its `InsertLoadJob` before this
call. If `waitForAutoStart()` throws (for example on manual shutdown, an RPC
error, or resume timeout), execution never enters the following `try`/`catch`,
so `onFail()` and `recordFinishedLoadJob()` are skipped. The job remains
`PENDING`, and incomplete load jobs never expire, so retries accumulate
permanent `SHOW LOAD` entries and memory. Include this wait in the executor
cleanup scope (or perform it before registration), and cover the throwing path.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertIntoTableCommand.java:
##########
@@ -354,6 +357,13 @@ public void beforeComplete(AbstractInsertExecutor
insertExecutor, StmtExecutor e
throw new AnalysisException("Insert plan failed. Could not get target
table lock.");
}
Review Comment:
[P1] Share the pre-wake with every dummy-location caller
This helper only covers normal `InsertIntoTableCommand`. Two other
user-facing paths reach the changed nonblocking snapshot without it:
transactional constant `INSERT VALUES` goes through
`BatchInsertIntoTableCommand` and plans the sink before
`selectBackendForTxnLoad()`, while cloud broker load initializes
`NereidsLoadingTaskPlanner` without waking its saved compute group. For an
empty automatic-partition table assigned to a suspended group, both paths find
no load-available dummy backend and fail before later backend/coordinator
selection can resume the group. Move the resolve/wake step to shared pre-lock
entry points for these callers, and add suspended-group coverage for
transactional values and broker load.
--
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]
