brpc dns cache#63546
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
I found one blocking issue: the BE test mock override was not updated after changing ExchangeSinkBuffer::_failed to take Status, so BE test builds that include exchange_sink_test.h will fail to compile.
Critical checkpoint conclusions:
- Goal and correctness: The PR aims to refresh DNS/cache state on BRPC network faults and validate rebuilt cached clients with handshake. The core runtime path is directionally consistent, but the change is not build-correct for BE tests.
- Scope: The runtime changes are mostly focused on BRPC client/DNS cache behavior, with small call-site updates for original hostnames.
- Concurrency: DNS dirty/cache access is protected by the existing shared_mutex in the changed paths; I did not find a new lock-order issue in the reviewed diff.
- Lifecycle/static initialization: No new cross-TU static initializer dependency was found.
- Configuration: Two mutable configs are added; dynamic reads are used at client lookup time. The config default enables the new handshake path, so this needs successful build/test coverage before merge.
- Compatibility: No persisted format or FE/BE thrift schema change was introduced. The handshake RPC already exists on the affected stubs.
- Parallel paths: Direct no-cache BRPC call sites touched by the PR pass the original hostname where available; cached get_client paths are centralized.
- Tests: No test evidence is provided in the PR description, and the existing BE exchange test mock is currently broken by the signature change.
- Observability: The changed paths log DNS invalidation, network faults, and handshake failures; no additional metrics appear strictly required for this patch.
- Transactions/data writes: No direct transaction or storage visibility changes were introduced.
- Performance: The new default synchronous handshake on cache misses can add latency, but I did not identify a separate blocking correctness issue beyond the compile break.
User focus points: no additional user-provided review focus was present.
| #else | ||
| virtual void _ended(RpcInstance& ins); | ||
| virtual void _failed(InstanceLoId id, const std::string& err); | ||
| virtual void _failed(InstanceLoId id, Status err); |
There was a problem hiding this comment.
This signature change breaks the BE test mock override. be/test/exec/exchange/exchange_sink_test.h still declares void _failed(InstanceLoId id, const std::string& err) override, so when tests are built with BE_TEST the override no longer matches ExchangeSinkBuffer::_failed(InstanceLoId, Status) and compilation will fail. Please update the mock (and any other BE_TEST overrides) to take Status, or avoid changing the virtual test-only signature.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)