HBASE-29555 TestRollbackSCP.testFailAndRollback fails sometimes because non empty Scheduler queue#8357
Open
SwaraliJoshi wants to merge 3 commits into
Open
HBASE-29555 TestRollbackSCP.testFailAndRollback fails sometimes because non empty Scheduler queue#8357SwaraliJoshi wants to merge 3 commits into
SwaraliJoshi wants to merge 3 commits into
Conversation
…se non empty Scheduler queue TestRollbackSCP restarts the master ProcedureExecutor in place, reusing the same executor and MasterProcedureScheduler instances to simulate a failover. While the executor is being reloaded, other still-running threads of the live mini-cluster master can push a procedure back into the shared scheduler in the window between clearing it and ProcedureExecutor.load() asserting that it is empty, so load() intermittently fails with "scheduler queue not empty". There are two such producers: - the asyncTaskExecutor callback that wakes a procedure after an async meta update (e.g. persistToMeta), and - an incoming reportRegionStateTransition RPC from a live region server, handled on an RpcServer thread, which wakes a procedure via ProcedureEvent. Fix ProcedureTestingUtility.restart() (test-only): - wait for the already shutdown asyncTaskExecutor to fully terminate before clearing the scheduler, so any pending wake-up callback has finished; and - reload (clear -> store start -> init) in a bounded retry loop, retrying only when load() fails because the scheduler is not empty. ProcedureExecutor.stop() is explicitly safe to call after a failed init(), so this is a clean redo. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
I prefer we just change the code the TestRollbackSCP. We have lots of other tests which rely on this restart, changing the logic may hide some other bugs we want to expose in these tests. Thanks. |
Contributor
Author
@Apache9 Yes, I am checking if we have missed any condition to stimulate the real production restart of master due to which this issue is happening only in the test. Please allow me sometime to make the necessary changes. |
| // scheduler is not empty, stop/drain/clear again and retry. ProcedureExecutor.stop() is | ||
| // explicitly safe to call after a failed init(), so this is a clean redo of the reload. | ||
| final int maxRestartAttempts = 20; | ||
| for (int attempt = 1;; attempt++) { |
Contributor
There was a problem hiding this comment.
Emptying scheduler is not out goal here. Check cause of this and try to resolve that. If we are missing something while simulating the Master restart lets add that.
…ailover
Fix the flaky TestRollbackSCP.testFailAndRollback ("IllegalArgumentException:
scheduler queue not empty" from ProcedureExecutor.load()) entirely within the
test, by making the in-place procedure-executor reload reproduce the two
guarantees a real master failover provides instead of relying on a retry:
- Gap #1: after stopping the executor, wait for its asyncTaskExecutor to fully
terminate before reloading, so a pending async meta-update wake-up cannot
re-populate the scheduler during load(). A real failover starts a fresh
process, so no such callback survives.
- Gap apache#2: hold a fair ReentrantReadWriteLock write lock across the reload, and
have reportRegionStateTransition take the read lock via non-blocking tryLock,
rejecting reports with PleaseHoldException during the reload (the region
server retries). This mirrors MasterRpcServices.checkServiceStarted(), which
rejects region reports until the master finishes initializing, i.e. after
load(). Acquiring the write lock also waits for any in-flight report.
The shared ProcedureTestingUtility.restart() is left unchanged so the other
tests that rely on it keep their strict scheduler-empty invariant.
Co-authored-by: Cursor <cursoragent@cursor.com>
Apache9
reviewed
Jun 30, 2026
| // reporting region state transitions. See restartMasterProcedureExecutorLikeFailover(). Fair so | ||
| // that, while the reload is waiting for / holding the write lock, new reports are rejected rather | ||
| // than barging in. A region state report takes the read lock; the reload takes the write lock. | ||
| private static final ReentrantReadWriteLock RELOAD_LOCK = new ReentrantReadWriteLock(true); |
Contributor
There was a problem hiding this comment.
Actually I do not get the point why here we need fair lock?
| // reload; acquiring the write lock in the reload still waits for any already in-flight report. | ||
| boolean locked = false; | ||
| try { | ||
| locked = RELOAD_LOCK.readLock().tryLock(0, TimeUnit.SECONDS); |
Contributor
There was a problem hiding this comment.
What does timeout 0 means here? No timeout or same with tryLock without any parameters?
| } | ||
| long deadline = System.currentTimeMillis() + 60000; | ||
| try { | ||
| while (!asyncTaskExecutor.awaitTermination(1, TimeUnit.SECONDS)) { |
Contributor
There was a problem hiding this comment.
Why not just wait for 1 minute?
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
3 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.
Summary
TestRollbackSCP.testFailAndRollbackis flaky: it intermittently fails withjava.lang.IllegalArgumentException: scheduler queue not emptyfromProcedureExecutor.load()while restarting the master procedure executor.The test restarts the
ProcedureExecutorin place, reusing the sameexecutor and
MasterProcedureSchedulerinstances to simulate a failover. Whilethe executor is being reloaded, other still-running threads of the live
mini-cluster master can push a procedure back into the shared scheduler in the
small window between
scheduler.clear()andProcedureExecutor.load()'sPreconditions.checkArgument(scheduler.size() == 0, ...).Two producers were identified:
asyncTaskExecutorcallback that wakes a procedure after an async metaupdate (e.g.
AssignmentManager.persistToMeta), andreportRegionStateTransitionRPC from a live region server,handled on an
RpcServerhandler thread, which wakes a procedure throughProcedureEvent.wake->scheduler.addFront.This is a test-infrastructure issue: a real master failover starts a fresh
process with a fresh executor/scheduler, so the production
load()preconditionis not affected. The fix is therefore confined to test code.
Solution (
ProcedureTestingUtility.restart(), test-only)The fix makes the test's in-place reload faithfully reproduce a real master failover by closing the two gaps, with no retry loop.
Gap 1 — no stale async wake survives the reload (awaitAsyncTaskExecutorTermination)
A new helper that, after the executor is stopped (which shuts the asyncTaskExecutor down), waits for that executor to fully terminate (bounded 60s) before the reload clears the scheduler. This guarantees any pending persistToMeta/regionClosedAbnormally wake-up callback has finished, and any future completing afterward is rejected by the shut-down executor. It reproduces production's guarantee that "the old process's async work is gone before load()."
Gap 2 — no region report during the reload (RELOAD_LOCK + report override)
A fair ReentrantReadWriteLock RELOAD_LOCK.
AssignmentManagerForTest.reportRegionStateTransition(...) is overridden to take the read lock via non-blocking tryLock; if the reload holds the write lock it throws PleaseHoldException (the RegionServer retries). This mirrors production exactly — MasterRpcServices.checkServiceStarted() rejects reports until the master finishes initializing (which is after load()). tryLock (not a blocking lock) is deliberate: it makes a report never block, so the AM re-init inside the reload can't deadlock behind a blocked report, and RPC handler threads stay free.
Test plan
TestRollbackSCP100x consecutively with the fix: 100/100 passed (twice).hbase-proceduretests and a representativehbase-serversubset (TestSCP, TestProcedureAdmin,TestTransitRegionStateProcedure, TestCreateTableProcedure): all pass.