Conversation
Transaction INSERT execution failures raised directly while dispatching fragments bypass ErrorReport. OlapTxnInsertExecutor catches these failures and aborts the subtransaction, but previously left QueryState successful, causing the client to receive Query OK. Set the INSERT error state when no earlier error exists so the failure reaches the client while preserving errors already reported by ErrorReport.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
There was a problem hiding this comment.
🔵 Needs a closer look
Existing query error details can be overwritten by unconditional fallback handling in two executors.
Pull request overview
Fixes FE transaction INSERT failures being reported as successful queries and corrects cloud subtransaction abort metadata.
Changes:
- Propagates transaction insert failures to clients.
- Centralizes insert error-state handling.
- Adds fragment failure regression coverage.
- Corrects the cloud subtransaction count.
File summaries
| File | Summary | Review finding |
|---|---|---|
regression-test/suites/insert_p0/transaction/txn_insert_inject_case.groovy |
Adds fragment execution failure coverage. | — |
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/OlapTxnInsertExecutor.java |
Reports transaction insert failures. | Moderate: preserve existing ERR state and message instead of unconditionally setting ERR_UNKNOWN_ERROR (1 vote). |
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/OlapInsertExecutor.java |
Centralizes insert error handling. | Moderate: only apply fallback error handling when the query state is not already ERR (2 votes). |
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/OlapGroupCommitInsertExecutor.java |
Reuses shared error handling. | — |
fe/fe-core/src/main/java/org/apache/doris/cloud/transaction/CloudGlobalTransactionMgr.java |
Sends the correct subtransaction count. | — |
Review details
Suppressed comments (2)
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/OlapInsertExecutor.java:318
ErrorReport.reportDdlException()sets the query state toERRbefore throwing, andexecuteSingleInsertthen routes that throwable throughonFail; this helper currently overwrites that existing code/message withERR_UNKNOWN_ERROR. That loses the original timeout/coordinator error instead of preserving it as described in the PR. Only set the fallback error when the state is not alreadyERR.
protected void setErrorState() {
String firstErrorMsgPart = "";
String urlPart = "";
if (!Strings.isNullOrEmpty(coordinator.getFirstErrorMsg())) {
firstErrorMsgPart = StringUtils.abbreviate(coordinator.getFirstErrorMsg(),
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/OlapTxnInsertExecutor.java:90
- This fallback is unconditional, so it overwrites any error already stored in
QueryState(both the specific error code and message) withERR_UNKNOWN_ERROR. That loses diagnostics from an earlierErrorReport/completion path, contrary to the intended preservation behavior; only callsetErrorState()whenctx.getState().getStateType() != MysqlStateType.ERR.
setErrorState();
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
pick #67386