HBASE-29555 TestRollbackSCP.testFailAndRollback fails sometimes because non empty Scheduler queue#8357
HBASE-29555 TestRollbackSCP.testFailAndRollback fails sometimes because non empty Scheduler queue#8357SwaraliJoshi 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>
|
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. |
@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++) { |
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.
There was a problem hiding this comment.
@Umeshkumar9414 Please re-review now. I have made the necessary changes.
…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>
| // 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); |
There was a problem hiding this comment.
Actually I do not get the point why here we need fair lock?
There was a problem hiding this comment.
Fairness is there to prevent the reload thread (the writer) from being starved by a steady stream of region-state reports (the readers). The report path uses the timed tryLock(0, …), which honors the fairness policy: with a fair lock, once the reload is queued waiting for the write lock, new tryLock attempts return false and the report is rejected (PleaseHoldException, RS retries), so the writer acquires promptly. With a non-fair lock, readers "barge" ahead of the waiting writer, so under continuous reports the reload could be delayed.
Fairness prevents reload starvation under report load, happy to drop it if you think that's over-engineering.
| // 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); |
There was a problem hiding this comment.
What does timeout 0 means here? No timeout or same with tryLock without any parameters?
There was a problem hiding this comment.
Per the JDK docs, the no-arg tryLock() always "barges" — it grabs the read lock if the write lock isn't currently held, ignoring fairness even on a fair lock. The timed tryLock(0, unit) honors the fairness policy — on a fair lock it will refuse to acquire if a writer is already waiting. That difference is precisely why the code uses tryLock(0, …) together with the fair lock: it's non-blocking (a report never blocks → no deadlock, RPC handler threads stay free) and it respects the waiting reload writer (so reports get rejected as soon as the reload starts). If you used the no-arg tryLock(), the fair flag would be meaningless (it would barge anyway).
Fair lock + timed tryLock(0) = non-blocking and fairness-honoring; that combination is what makes reports reject-and-retry during reload without starving the reload.
| } | ||
| long deadline = System.currentTimeMillis() + 60000; | ||
| try { | ||
| while (!asyncTaskExecutor.awaitTermination(1, TimeUnit.SECONDS)) { |
There was a problem hiding this comment.
Why not just wait for 1 minute?
There was a problem hiding this comment.
You are right, let me change it to 1 minute.
3 participants
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.