Apache9 commented on code in PR #8357:
URL: https://github.com/apache/hbase/pull/8357#discussion_r3499870322
##########
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);
Review Comment:
Actually I do not get the point why here we need fair lock?
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRollbackSCP.java:
##########
@@ -160,6 +203,67 @@ public void describeTo(Description description) {
};
}
+ /**
+ * Wait for the (already shutdown) asyncTaskExecutor of the given executor
to fully terminate.
+ * HBASE-29555 (Gap #1): callbacks that wake a procedure after an async
operation (e.g.
+ * AssignmentManager#persistToMeta run on the executor's asyncTaskExecutor,
not on a
+ * PEWorker. In production a master restart is a fresh process, so no such
callback can survive
+ * into the reloaded executor. Here we reuse the same executor in-place, so
a pending callback can
+ * call scheduler.addFront during the reload. ProcedureExecutor shuts the
+ * asyncTaskExecutor down; waiting for it to terminate guarantees any
pending callback has run
+ * before we clear the scheduler, reproducing the "no async work survives
the restart" guarantee.
+ */
+ private static void awaitAsyncTaskExecutorTermination(ProcedureExecutor<?>
procExec) {
+ ExecutorService asyncTaskExecutor = procExec.getAsyncTaskExecutor();
+ if (asyncTaskExecutor == null) {
+ return;
+ }
+ long deadline = System.currentTimeMillis() + 60000;
+ try {
+ while (!asyncTaskExecutor.awaitTermination(1, TimeUnit.SECONDS)) {
Review Comment:
Why not just wait for 1 minute?
##########
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:
What does timeout 0 means here? No timeout or same with tryLock without any
parameters?
--
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]
