CalvinKirs opened a new pull request, #70348: URL: https://github.com/apache/doris/pull/70348
### Background When a client connects to a non-master FE, commands that require the master are forwarded there. After execution and journal synchronization, local callbacks update the connected session for commands such as SET and UNSET. ### What problem does this PR solve? Issue Number: None Related PR: #41913 (introduced the Nereids post-forward command callback) PR Type: Bugfix Problem Summary: A rejected forwarded SET can still change variables on the connected FE. For example, `SET GLOBAL version_comment = 'rejected_forward'` reports that the variable is read-only, but the follower's local callback still changes its value. Skip local callbacks when the master returns ERR and preserve the master's error response. Most of the diff adds regression tests for rejected and successful SET commands. ### Details - **Frequency:** Always for the reproduced rejected SET GLOBAL version_comment on a follower when the forwarding RPC and journal wait complete normally. - **Root cause:** `FEOpExecutor.execute()` retains the master's SQL error as a normal RPC result. `StmtExecutor.forwardToMaster()` then unconditionally runs the local callback, which can bypass the master's read-only check through `VariableMgr.setVarForNonMasterFE()`. - **Why existing tests missed it:** Unit tests such as `VariableMgrTest.testReadOnly` check direct rejection, while `FEOpExecutorMysqlProtocolTest` checks forwarded packet handling; neither combines a failed master result with the real local variable callback. Regression suites `set_and_unset_variable_command` and `test_set_global_default` exercise normal variable changes and defaults, without asserting that a rejected forwarded global SET leaves the follower's value unchanged. - **Fix:** Check the existing SQL status at the common forwarding boundary and return before success callbacks on ERR. Keep the saved result, error message and response packet for the existing response path. - **Other paths and branches:** SET and UNSET share this boundary. Successful commands and EOF/UNKNOWN statuses retain their behavior. Freshly fetched branch-4.0, branch-4.1 and branch-4.2 contain the same unguarded Nereids callback and need backports; branch-3.1 has analogous legacy SET/UNSET synchronization requiring adaptation. This PR targets master only. ### Testing - Ran `bash run-fe-ut.sh --run org.apache.doris.qe.StmtExecutorForwardFailureTest` locally before and after the fix. Before: 2 tests, 1 failure because version_comment changed to rejected_forward despite the rejection. After: 2 tests passed, with no failures, errors or skipped tests. - The negative test checks the unchanged real variable and retained error status/message/packet; the positive test checks successful session synchronization. The remote executor is mocked; no post-fix multi-FE end-to-end run was performed. ### Notes for Reviewers Use the string ERR status rather than a nonzero numeric status code: successful EOF responses can also carry a nonzero numeric code. ### Release note Failed forwarded SET and UNSET commands no longer run local callbacks that could change variables on the connected FE despite the reported error. ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Codex (GPT-6) - Generated: Production fix, FE unit tests, and this description. - Human review: not stated -- 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]
