[fix](be) Avoid repeated peer connect failures - #67464
Open
deardeng wants to merge 1 commit into
Open
Conversation
Problem: Peer reads create an uncached BRPC client for every request. When a peer is unreachable, readers across tablets repeatedly connect to the same address with the default 2-second connect timeout and 10 retries. Impact: An unavailable cache peer can repeatedly delay reads before remote-storage fallback and generate unnecessary connection attempts and warning logs. Fix: - Add a BE-wide, address-level circuit breaker shared by peer readers. - Open the circuit after a configurable number of consecutive connection or RPC failures (3 by default) for a configurable cooldown (30 seconds by default). - Reject reads while the circuit is open, allow one probe after cooldown, and clear the failure state after a successful RPC. - Make uncached BRPC connect timeout and retry count configurable at the call site, and use a 200 ms timeout with no retries for peer reads. Test: Stop the peer test server and verify that four reads issue only three RPC attempts, with the final read rejected by the circuit breaker.
deardeng
requested review from
Gabriel39,
gavinchou,
liaoxin01 and
morningman
as code owners
September 3, 2026 03:40
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16864 ms |
Contributor
TPC-DS: Total hot run time: 81783 ms |
Contributor
ClickBench: Total hot run time: 14.59 s |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Overall: the PR addresses the repeated-connect latency case, and the new BRPC defaults preserve existing callers, but the address circuit-breaker state is not correctly integrated with peer-candidate recovery and mutable configuration.
Blocking findings:
- Circuit-open returns generic THRIFT_RPC_ERROR, so both peer-read paths evict candidates and count all-miss attempts even though no RPC was made; this can remove the candidate before the half-open probe.
- Transport-successful INTERNAL_ERROR responses clear the address failure state before status validation, so repeated RPC failures do not trip the advertised BE-wide breaker.
- The process-wide health map has no expiry or bound; permanently dead/retired endpoints accumulate until BE shutdown.
- Mutable threshold/cooldown changes are not reconciled with stored deadlines; raising the threshold after a circuit opens can turn every post-expiry failure into an immediate probe with no cooldown.
Review checkpoints:
- Goal/test: the connection-refusal path is covered by the new unit test and CI BE UT is green, but cooldown, mutable-config, non-OK-status, and state-cleanup behavior are not covered.
- Scope: the diff is focused, but the caller-side candidate lifecycle must be updated with the new circuit status.
- Concurrency: the bthread mutex protects map transitions and the single half-open probe; no separate lock-order issue was found, but the state transition flaws above remain under concurrent readers.
- Lifecycle/memory: global state survives candidate expiry and is not bounded.
- Configuration: both new fields are mutable, yet existing circuit deadlines and probe state are not adjusted on runtime updates.
- Compatibility: all existing get_new_client_no_cache callers retain the prior 2000 ms connect timeout and 10 retries; only peer reads use 200 ms and zero retries.
- Parallel paths: both peer race and sequential fallback paths share the circuit-open misclassification.
- Data/error safety: reads fall back to S3 rather than corrupting data, but peer failure classification and recovery are incorrect.
- User focus: no additional focus was provided; the complete changed-file review is covered above.
Please address the inline findings and add tests for status classification, cooldown/probe transitions after config updates, and bounded health-state cleanup.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem:
Peer reads create an uncached BRPC client for every request. When a peer is unreachable, readers across tablets repeatedly connect to the same address with the default 2-second connect timeout and 10 retries.
Impact:
An unavailable cache peer can repeatedly delay reads before remote-storage fallback and generate unnecessary connection attempts and warning logs.
Fix:
Test:
Stop the peer test server and verify that four reads issue only three RPC attempts, with the final read rejected by the circuit breaker.
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)