Improve Failure Detector on Broker. #230
Open
siddharthteotia wants to merge 1 commit into
Open
Conversation
…ed recovery probe with probation - Detect and quarantine a silent-but-connected server after N consecutive no-response queries (notifyServerResponded/notifyServerNotResponded), instead of waiting for a transport-level channelInactive/reset. - Replace the bare TCP-connect recovery probe with an HTTP /health/readiness probe so a half-dead server (kernel completes the handshake but cannot serve) is not re-admitted. - Add a probation window: require N consecutive successful probes before re-admitting and do not reset the exponential backoff on a single success, eliminating flapping. Behavior uses hardcoded best-effort defaults (consecutive-failures threshold = 5, healthy threshold = 3; probation re-probe reuses the retry initial delay) - no new configs added. Tests: ConnectionFailureDetectorTest, SingleConnectionBrokerRequestHandlerHealthProbeTest, FailureDetectorIncidentSimulationTest. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
c8adb5d to
44305eb
Compare
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
JIRA - https://linkedin.atlassian.net/browse/OA-1693
Timeline
Pinot error code: 427. 1 servers [<host>] not responded. Internal server error for query=blahCaught exception while handling response from server: lva2-app27368_O java.net.SocketException: Connection resetDataTableHandler.exceptionCaught()followed bychannelnactive()listener marking the server down.Channel for server: <host> is now inactive, marking server downThe 2 events at T + 3mins continue to repeat for about 30secs.
T = T + 3mins
T = T + 3mins 30secs.
After every 1.5 to 2 hours, the entire pattern gets repeated (from what we observed originally at T). In that gap of 1.5/2hours, the server is processing queries. Separate investigation on what happened at server.
Impact
Root Cause Analysis
Current state machine has gaps.
PHASE 1 — server goes silent , socket still established
TL;DR - No failure-detector call. Server stays in routing. It will be queried again.
PHASE 2 — socket finally reset
TL;DR- Server gets excluded from routing.
PHASE 3 — recovery probe (every ~5s) → FLAP
TL;DR - Eagerly re-admits after a TCP handshake succeeds. A server may be half-dead (not ready to serve queries) but the kernel will still be able to ACK the SYN. The flapping loop of UNHEALTHY -> HEALTHY-> UNHEALTHY -> HEALTHY is persistently tried with 5secs retry_delay since the eager re-admission does not preserve the back-off.
Flap demonstration in the existing (old) code against the half-dead server (whose TCP connect always succeeds). Nominal D = 5s initial delay, no jitter in the example for clarity (jittering is applied in the existing code)
Solution
In the new state machine, upon receiving a 427 (server not responded), we invoke FailureDetector's
notifyServerNotResponded. Upon 5 such consecutive events, failure detector pulls the server from routing as opposed to -- letting the queries be routed to it (receive 427) and wait for the socket connection reset trigger to invoke the failure detector.Similarly on the recovery side, improved the health check probe to layer HTTP health readiness call on top of current QueryRouter.connect() (which succeeds after TCP ack).
Furthermore, the back-off delay is preserved in a way to prevent FLAP as much as possible. See the example timelines below to understand the recovery mechanics.
No new configs have been added.
In the actual problematic scenario we saw in production (uptime SLO breach), the solution would lead to substantially fewer failed requests, and the impact duration drops from ~5 minutes → ~seconds. The error-budget burn per occurrence shrinks by a large factor. The bad server is taken out of routing quickly (detection is better) and doesn't flap back, letting the broker serve from the healthy replica groups and protecting the uptime SLO. The residual failures are a small.
Recovery examples
TL;DR -
If you see the table in RCA section, every cycle destroys and recreates the RetryInfo → the delay is reborn at 5s. The exponential growth ( _retryDelayNs *= factor ) only happens when the probe fails branch but here the probe succeeds every time (TCP connect lies) and the RetryInfo is discarded on re-admit, so growth never accumulates. So pinned at 5s → re-admit ↔ fail flap every ~5s for the whole outage window.
In this PR, the server is re-admitted only after 3 consecutive successful probes, and healthy check probe successes don't discard/reset the RetryInfo.
NOTE - Jittering existed in retry code and is left unchanged.
Scenario A — clean recovery (nominal, no jitter for simplicity):
Scenario B — success interrupted (the anti-flap case, both successes and failures)
Testing
Follow-Ups
I noticed that entire failure dector code is not as clean as it can be. Abstractions are not clear + duplication of responsibilities. Refactor markServerDown vs markServerUnhealthy into a single explicit FailureDetector event surface, instead of the implicit getFailedServer() poll bridging three call sites.
Retry jitter: the pre-existing jitter is one-sided (only shortens). Consider a mean-preserving without shortening the effective backoff.