HADOOP-19987. ObserverReadProxyProvider's probing pool blocks when saturated instead of rejecting - #8724
Open
joseluisll wants to merge 1 commit into
Open
HADOOP-19987. ObserverReadProxyProvider's probing pool blocks when saturated instead of rejecting#8724joseluisll wants to merge 1 commit into
joseluisll wants to merge 1 commit into
Conversation
…turated instead of rejecting getHAServiceStateWithTimeout catches RejectedExecutionException and returns null so the caller falls back to the active NameNode. That fallback cannot be reached. The pool was a BlockingThreadPoolExecutorService, which wraps the real executor in a SemaphoredDelegatingExecutor whose submit() blocks on queueingPermits.acquire() rather than rejecting, so it never throws RejectedExecutionException. A caller that should have been shed onto the active NN is instead parked -- while holding this provider's monitor, since the probe is reached through the synchronized changeProxy(), so every other thread needing that monitor waits behind it. This is latent rather than something users hit today. The one production caller runs inside the synchronized changeProxy(), so a provider has at most one probe in flight, and dfs.client.failover.namenode.ha-state.probe.timeout defaults to 0, which makes task.get() wait indefinitely. Saturation needs that timeout set above 0, a NameNode whose RPC does not return on interrupt so that cancelled probes keep occupying the four threads, and enough further probes to fill the queue behind them. The consequence when it does happen is severe and permanent: the provider's monitor is held by a thread that will never make progress. Use a plain ThreadPoolExecutor with the same shape (4 threads, 128-deep queue, 10s idle timeout via allowCoreThreadTimeOut, daemon threads) and the default AbortPolicy, so saturation reaches the fallback written for it. Capacity is unchanged: the semaphore permitted activeTasks + waitingTasks, the same 132 as four threads plus a 128-deep queue. The field type widens to ExecutorService. Under saturation the provider now sheds load onto the active NN rather than applying backpressure, which is what the fallback was written to do. Thread names keep their nn-ha-state-probing-pool<N>-t<M> shape, so a dump still says which provider a thread belongs to in a JVM holding one per nameservice. Two details shift: the per-thread counter starts at t0 rather than t1, and <N> now counts probing pools alone, where before it came from a counter BlockingThreadPoolExecutorService shares among all of its users. Adds two tests. Against the old pool both fail, blocking for the full 20s deadline: submit past capacity did not return within 20000ms: it blocked instead of failing fast HA state probe on a saturated pool did not return within 20000ms: it blocked instead of failing fast Each runs the call under test on its own thread with an explicit deadline: a bare @timeout would catch the hang but report only "timed out after N seconds" without naming the stuck call. The pool and its sizing are exposed @VisibleForTesting so the test can fill it exactly, which also removes the magic numbers the constructor previously hardcoded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@pan3793 @slfan1989 @Hexiaoqiao This PR is Green and ready to be reviewed. It is a prerrequisite for HADOOP-19979, that will fix 4 flaky conditions so that we reduce later the GHA exclude list, as explained on HADOOP-19981 [umbrella for all GHA reactivation candidates from excluded-tests.txt] and its subtasks [specific identified candidates for reactivation]. |
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
2 participants
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.
Description of PR
https://issues.apache.org/jira/browse/HADOOP-19987
getHAServiceStateWithTimeoutcatchesRejectedExecutionExceptionto fall back to the active NameNode, but that fallback is unreachable: the probing pool was aBlockingThreadPoolExecutorService, whosesubmit()blocks on a semaphore instead of rejecting. A caller that should have been shed onto the active NN is parked instead — while holding the provider's monitor, since the probe runs inside thesynchronized changeProxy().Latent today: the only production caller is inside
changeProxy(), so there is at most one probe in flight per provider. Reaching saturation needsdfs.client.failover.namenode.ha-state.probe.timeoutset above its0default plus probes that don't return on interrupt — but when it happens the monitor is held forever.Fix: use a plain
ThreadPoolExecutorwith the same shape (4 threads, 128-deep queue, 10s idle timeout, daemon threads) and the defaultAbortPolicy. Capacity is unchanged at 132; the field type widens toExecutorService. Thread names keep thenn-ha-state-probing-pool<N>-t<M>shape, with the per-thread counter now starting att0and<N>counting probing pools alone.How was this patch tested?
Two new tests in
TestObserverReadProxyProvider, each running the call under test on its own thread with an explicit deadline so a hang names the stuck call:testProbingPoolRejectsOnceSaturated—submit()past capacity throws instead of blocking.testFallsBackToActiveWhenProbingPoolSaturated— the HA-state probe returns and falls back to the active NN.Both fail against the old pool, blocking for the full 20s deadline.
Full CI green on the fork (
common,hdfs - other,hdfs - slow,hdfs-rbf,mr,other,yarn-server-rmon Java 17; build-only on Java 21 and 25): https://github.com/joseluisll/hadoop/actions/runs/34147776162commonneeded a rerun forTestZKDelegationTokenSecretManager#testNodesLoadedAfterRestart, an unrelated pre-existing flake inhadoop-common; this patch only toucheshadoop-hdfs-client.For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?AI Tooling
Contains content generated by Claude Code.