Doris-Breakwater commented on issue #69984: URL: https://github.com/apache/doris/issues/69984#issuecomment-6573747702
The reported recovery gap is supported by source inspection and merits a BE RPC/DNS bug fix. I checked the local `4.1.4` tag (`ad35a140c7fd0b842f18c23300bac581f7d04326`) and the cited master revision `0fe5cc1e31924a69528b0661db91979a3eda526c`. This is a source-based assessment; I have not reproduced the pod restart or verified the unpublished patch/ASAN results. The issue currently has no labels; BE networking/RPC and load availability are the relevant triage areas. The verified sequence is: 1. `DNSCache::get()` returns an existing positive entry without calling the resolver. In `4.1.4`, the background loop sleeps for one minute between refresh cycles, and a failed resolution retains the old IP. This permits the old address to survive a temporary DNS outage and remain in use after DNS has recovered. See [DNSCache](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/util/dns_cache.cpp#L36-L120). 2. `BrpcClientCache::get_client(host, port)` obtains that cached address, checks the existing stub, and rebuilds an unhealthy channel using the same address. It does not request fresh DNS resolution. Both the asynchronous closure and synchronous call path mark the Doris channel status unhealthy only for a final `EHOSTDOWN`; a final `ECONNREFUSED` alone does not set that status. This confirms the detection gap, but does not prove that IP reuse produced `ECONNREFUSED` in this incident. See [channel detection and cache lookup](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/util/brpc_client_cache.h#L69-L252). 3. `VNodeChannel` obtains its stub using `_node_info.host` and `_node_info.brpc_port`. After an open RPC failure, if the handshake availability check also fails, `open_wait()` erases by `remote_side()`. For a hostname-keyed entry, that numeric endpoint does not remove the intended entry. Correcting the erase key alone still leaves the stale DNS entry. See [stub acquisition](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/exec/sink/writer/vtablet_writer.cpp#L613-L619) and [failure handling](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/exec/sink/writer/vtablet_writer.cpp#L829-L855). One version qualification matters for the PR: master already includes interruptible condition-variable waits, authoritative-failure eviction, and negative caching from [#63363](https://github.com/apache/doris/pull/63363). Positive cache hits still bypass resolution, and unhealthy-channel reconstruction still lacks a refresh, so that work does not close this recovery gap. Please preserve those existing protections rather than replace them with the simpler `4.1.4` implementation. See [master DNSCache](https://github.com/apache/doris/blob/0fe5cc1e31924a69528b0661db91979a3eda526c/be/src/util/dns_cache.cpp#L36-L115) and [refresh/eviction loop](https://github.com/apache/doris/blob/0fe5cc1e31924a69528b0661db91979a3eda526c/be/src/util/dns_cache.cpp#L313-L373). The periodic recovery reported here also does not establish the persistent load-stream socket failure described in [#64708](https://github.com/apache/doris/issues/64708). The proposed failure-triggered, per-host rate-limited refresh and hostname-based invalidation address the verified mechanism. Please submit the implementation and tests for review, with these checks: - **Concurrency/performance:** reserve the per-host refresh attempt before resolving, rate-limit unsuccessful attempts too, and perform blocking resolution outside the DNS-cache and stub-map locks. Test overlapping periodic refresh, explicit refresh, invalidation, and stub creation so a delayed result or failure cannot overwrite/evict a newer healthy entry. Keep healthy cache hits inexpensive. - **Error handling and parallel paths:** cover `EHOSTDOWN` and `ECONNREFUSED` in both synchronous and asynchronous paths; preserve failed-open reporting. `LoadStreamStub::open()` also uses this cache synchronously, so test it or the equivalent shared path. Audit other endpoint-based erase callers; `DeltaWriter` has a similar pattern. These are additional review targets, not confirmed contributors to this report. - **DNS policy and lifecycle:** retain cached-IP fallback on transient DNS failure and master’s eviction/negative-cache guarantees. Define how explicit refresh interacts with negative entries. Reuse master’s existing shutdown notification; a condition variable cannot interrupt an already-blocked resolver call. Shared stub references already held by callers also survive cache erasure. - **Configuration and compatibility:** document the new mutable interval, valid values, failed-attempt throttling, and runtime updates. The proposal needs no wire/storage-format or FE/BE-field change; verify that the actual patch preserves that scope and transaction failure semantics. Cache replacement should help subsequent acquisition, not silently replay an already-failed load. - **Tests and observability:** include deterministic old-IP → resolution failure → new-IP tests, hostname-key invalidation, both error codes/call modes, concurrent throttling, unchanged-IP recovery, literal-IP behavior, and master’s existing DNS tests. Attach test commands/results and a before/after restart run showing the contacted endpoint and first successful subsequent load. ASAN results alone do not establish concurrency correctness. For runtime confirmation, please attach the exact BE build commit and patch/PR link, plus timestamped logs from one sending BE covering the last old-IP resolution, pod readiness, failed opens with full `ErrorCode`/`ErrorText` and remote endpoint, cache update, and recovery. During that window, capture hostname resolution from the same sending pod using the system resolver, and connectivity to the new BE’s brpc port. This distinguishes Doris’s positive cache from resolver propagation/negative caching and listener readiness. Please also include the minimal table DDL, effective replica/write-quorum settings, writer mode, and Stream Load response. In this path, channel failure becomes load failure when `IndexChannel` exceeds the allowed replica failures; replication ≥ 2 alone does not establish that every affected load must fail. See [replica failure accounting](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/exec/sink/writer/vtablet_writer.cpp#L195-L247). The intended acceptance criterion should be recovery on a subsequent eligible acquisition once the sender’s resolver returns the new address and the peer accepts RPCs. Pod Ready alone cannot guarantee that, and the current one-minute sleep is not a strict 60-second upper bound because DNS work and RPC timing add delay. Breakwater-GitHub-Analysis-Slot: slot_db2e828f90b1 -- 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]
