bosswnx commented on PR #67310: URL: https://github.com/apache/doris/pull/67310#issuecomment-5463467022
Hi maintainers, I also submitted #67310 for this issue (with a deterministic 4-FE docker reproduction in the issue thread), and I'd like to share one coverage gap I found in this approach while cross-checking both fixes against my reproduction, in case it helps the decision. **1. The main scenario in #67297 never reaches the new retry code** The retry here is inside `catch (TTransportException)`. But the primary failure mode from the issue is an old master that is **degraded but still alive** (OOM-recovered, lost the election, MySQL/thrift ports still serving — LB health checks keep routing to it too). Forwarding to such a node **succeeds at the transport level** and returns the business error "The statement has been forwarded to master FE(...) and failed to execute because Master FE is not ready" — no `TTransportException` is thrown, so the rediscovery code never runs. This is exactly what my reproduction observes. **2. The rediscovery source is the same stale cache it tries to fix** When a transport exception *does* occur (old master fully dead), the code re-reads `ctx.getEnv().getMasterHost()/getMasterRpcPort()`. But `masterInfo` is only refreshed by journal replay (`OP_MASTER_INFO_CHANGE`) — which is precisely the thing that's lagging/stuck in this bug. In the stuck-replay window the re-read returns the same stale address, `newAddr.equals(feAddr)` is true, and no retry happens. To recover independent of journal replay, the discovery needs a different source (e.g. `Env.getHaProtocol().getLeader()` asking bdbje directly, or probing known FE thrift endpoints). **3. Retrying on a transport exception may be unsafe for non-idempotent statements** A transport failure is semantically ambiguous — the statement may already have executed on the target before the connection broke. The retry here runs before the existing `shouldNotRetry` check, so DDL/DML can execute twice. In #67310 the retry is restricted to an explicit, pre-execution NOT_MASTER rejection returned by the receiver (the receiver rejects before executing the statement), which is safe to retry even for non-idempotent statements; ambiguous transport failures are deliberately not retried. This matches constraint 4 from the triage analysis in the issue thread. **4. Minor observations** - The retry lives in `FEOpExecutor`, which is also used for calls that intentionally target specific non-master FEs (all-FE config propagation, cross-FE query kill); redirecting those to the master changes their semantics (constraint 3 in the triage analysis). - This PR also removes `getAuditStatisticsBackendIds()` from `FEOpExecutor`, which backs the audit statistics feature recently added upstream — that looks unintentional and would regress it. - The `FORWARD_WITH_SYNC` hang (CREATE USER blocking in `JournalObservable.waitOn()` for up to `query_timeout * 1.2` on the stale `maxJournalId` returned by the old master) is not addressed; in #67310 a NOT_MASTER result skips the journal wait. Not claiming my approach is the only right one — happy to align or merge efforts if maintainers prefer a different direction. Just wanted to make sure the degraded-but-alive old master case (the one customers actually hit) is covered by whichever fix lands. -- 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]
