SwaraliJoshi commented on code in PR #8357:
URL: https://github.com/apache/hbase/pull/8357#discussion_r3584737119
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRollbackSCP.java:
##########
@@ -75,12 +85,45 @@ public class TestRollbackSCP {
private static final AtomicBoolean INJECTED = new AtomicBoolean(false);
+ // HBASE-29555: guards the in-place procedure-executor reload against region
servers concurrently
+ // 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);
+
private static final class AssignmentManagerForTest extends
AssignmentManager {
public AssignmentManagerForTest(MasterServices master, MasterRegion
masterRegion) {
super(master, masterRegion);
}
+ @Override
+ public ReportRegionStateTransitionResponse reportRegionStateTransition(
+ ReportRegionStateTransitionRequest req) throws PleaseHoldException {
+ // HBASE-29555 (Gap #2): in production a starting/recovering master
rejects region state
+ // reports (via HMaster.checkServiceStarted) until it has finished
initializing, which happens
+ // only after the procedure executor has loaded. The test instead
reloads the executor in place
+ // under a live master, so a report can wake a procedure and re-populate
the scheduler in the
+ // window between clearing it and ProcedureExecutor.load() asserting it
is empty. Mirror
+ // production: while the reload holds the write lock, reject reports
with PleaseHoldException
+ // (the region server retries). tryLock (non-blocking) is used so a
report never blocks the
+ // 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);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
