Copilot commented on code in PR #67386:
URL: https://github.com/apache/doris/pull/67386#discussion_r3903753184
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/OlapInsertExecutor.java:
##########
@@ -323,7 +328,6 @@ protected void onFail(Throwable t) {
}
String finalErrorMsg = InsertUtils.getFinalErrorMsg(errMsg,
firstErrorMsgPart, urlPart);
ctx.getState().setError(ErrorCode.ERR_UNKNOWN_ERROR, finalErrorMsg);
- recordLoadJob(ctx.getCurrentUserIdentity());
}
Review Comment:
`setErrorState()` depends on external mutable state (`errMsg`) rather than
the provided failure (`Throwable t`). This creates a hidden contract for
callers (they must ensure `errMsg` is set correctly beforehand) and makes reuse
from other executors more fragile. Consider changing `setErrorState` to accept
the message or `Throwable` (e.g., `setErrorState(Throwable t)` or
`setErrorState(String errMsg)`) and derive the local error message inside the
method to reduce coupling and prevent accidental stale/empty error messages.
##########
regression-test/suites/insert_p0/transaction/txn_insert_inject_case.groovy:
##########
@@ -40,6 +40,7 @@ suite("txn_insert_inject_case", "nonConcurrent") {
"""
}
GetDebugPoint().disableDebugPointForAllBEs("FlushToken.submit_flush_error")
+
GetDebugPoint().disableDebugPointForAllBEs("FragmentMgr.exec_plan_fragment.failed")
Review Comment:
The suite disables `FragmentMgr.exec_plan_fragment.failed` for all BEs at
setup time, but cleanup only disables it via per-BE HTTP calls. For symmetry
and to reduce dependence on `ipList/portList` completeness, consider adding
`GetDebugPoint().disableDebugPointForAllBEs(\"FragmentMgr.exec_plan_fragment.failed\")`
in the cleanup path as well (similar to the flush debugpoint block).
##########
regression-test/suites/insert_p0/transaction/txn_insert_inject_case.groovy:
##########
@@ -90,16 +91,30 @@ suite("txn_insert_inject_case", "nonConcurrent") {
assertTrue(e.getMessage().contains("dbug_be_memtable_submit_flush_error"))
}
- disableDebugPoint()
- sql """ insert into ${table}_0 select * from ${table}_1; """
+ disableDebugPoint("FlushToken.submit_flush_error")
sql """ commit"""
} catch (Exception e) {
logger.error("failed", e)
} finally {
sql """ rollback """
- disableDebugPoint()
+ disableDebugPoint("FlushToken.submit_flush_error")
GetDebugPoint().disableDebugPointForAllBEs("FlushToken.submit_flush_error")
}
+
+ try {
+ enableDebugPoint("FragmentMgr.exec_plan_fragment.failed")
+ sql """ begin """
+ test {
+ sql """ insert into ${table}_0 select * from ${table}_1; """
+ exception "FragmentMgr.exec_plan_fragment.failed"
+ }
+ disableDebugPoint("FragmentMgr.exec_plan_fragment.failed")
+ sql """ insert into ${table}_0 select * from ${table}_1; """
+ sql """ commit """
+ } finally {
+ sql """ rollback """
+ disableDebugPoint("FragmentMgr.exec_plan_fragment.failed")
Review Comment:
The suite disables `FragmentMgr.exec_plan_fragment.failed` for all BEs at
setup time, but cleanup only disables it via per-BE HTTP calls. For symmetry
and to reduce dependence on `ipList/portList` completeness, consider adding
`GetDebugPoint().disableDebugPointForAllBEs(\"FragmentMgr.exec_plan_fragment.failed\")`
in the cleanup path as well (similar to the flush debugpoint block).
--
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]
