From 9f9ee485239a40eb21b196faf962800b9ee48a29 Mon Sep 17 00:00:00 2001 From: huiruan <876107431@qq.com> Date: Sun, 4 Jun 2023 22:04:15 +0800 Subject: [PATCH 1/3] HBASE-27905 Directly schedule procedures that do not need to acquire locks --- .../hadoop/hbase/procedure2/Procedure.java | 11 ++++++ .../procedure/MasterProcedureScheduler.java | 36 ++++++++++++++++--- .../TestMasterProcedureScheduler.java | 24 +++++++++++++ 3 files changed, 66 insertions(+), 5 deletions(-) diff --git a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java index 43adba2bc21a..361bf6f1d838 100644 --- a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java +++ b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java @@ -272,6 +272,10 @@ protected boolean waitInitialized(TEnvironment env) { return false; } + public boolean needLock() { + return true; + } + /** * The user should override this method if they need a lock on an Entity. A lock can be anything, * and it is up to the implementor. The Procedure Framework will call this method just before it @@ -974,6 +978,9 @@ final LockState doAcquireLock(TEnvironment env, ProcedureStore store) { if (waitInitialized(env)) { return LockState.LOCK_EVENT_WAIT; } + if (!needLock()) { + return LockState.LOCK_ACQUIRED; + } if (lockedWhenLoading) { // reset it so we will not consider it anymore lockedWhenLoading = false; @@ -1000,6 +1007,10 @@ final LockState doAcquireLock(TEnvironment env, ProcedureStore store) { * Internal method called by the ProcedureExecutor that starts the user-level code releaseLock(). */ final void doReleaseLock(TEnvironment env, ProcedureStore store) { + if (!needLock()) { + return; + } + locked = false; // persist that we have released the lock. This must be done before we actually release the // lock. Another procedure may take this lock immediately after we release the lock, and if we diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureScheduler.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureScheduler.java index fbf0eb8abf32..69cc2011ab91 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureScheduler.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureScheduler.java @@ -149,19 +149,22 @@ private > void doAdd(FairQueue fairq, Queue queue, Procedure proc, boolean addFront) { queue.add(proc, addFront); // For the following conditions, we will put the queue back into execution - // 1. The procedure has already held the lock, or the lock has been restored when restarting, + // 1. The procedure does not need any lock at all. + // 2. The procedure has already held the lock, or the lock has been restored when restarting, // which means it can be executed immediately. - // 2. The exclusive lock for this queue has not been held. - // 3. The given procedure has the exclusive lock permission for this queue. + // 3. The exclusive lock for this queue has not been held. + // 4. The given procedure has the exclusive lock permission for this queue. Supplier reason = null; - if (proc.hasLock()) { + if (!proc.needLock()) { + reason = () -> proc + " does not need any lock"; + } else if (proc.needLock() && proc.hasLock()) { reason = () -> proc + " has lock"; } else if (proc.isLockedWhenLoading()) { reason = () -> proc + " restores lock when restarting"; } else if (!queue.getLockStatus().hasExclusiveLock()) { reason = () -> "the exclusive lock is not held by anyone when adding " + proc; } else if (queue.getLockStatus().hasLockAccess(proc)) { - reason = () -> proc + " has the excusive lock access"; + reason = () -> proc + " has the exclusive lock access"; } if (reason != null) { addToRunQueue(fairq, queue, reason); @@ -219,6 +222,9 @@ private > Procedure doPoll(final FairQueue fairq) // procedures, then we give up and remove the queue from run queue. for (int i = 0, n = rq.size(); i < n; i++) { Procedure proc = rq.poll(); + if (!proc.needLock()) { + return proc; + } if (isLockReady(proc, rq)) { // the queue is empty, remove from run queue if (rq.isEmpty()) { @@ -368,6 +374,13 @@ private static > void addToRunQueue(FairQueue fairq, private static > void removeFromRunQueue(FairQueue fairq, Queue queue, Supplier reason) { + if (hasNoLockNeededProcedure(queue)) { + if (LOG.isTraceEnabled()) { + LOG.trace("DO NOT remove {} from run queue because There are still procedures in the " + + "queue that do not need to acquire locks", queue); + } + return; + } if (LOG.isTraceEnabled()) { LOG.trace("Remove {} from run queue because: {}", queue, reason.get()); } @@ -376,6 +389,19 @@ private static > void removeFromRunQueue(FairQueue fa } } + private static > boolean hasNoLockNeededProcedure(Queue q) { + boolean ret = false; + // TODO: Iterate Queue in a more efficient way ? + for (int i = 0, n = q.size(); i < n; i++) { + Procedure proc = q.poll(); + if (!proc.needLock()) { + ret = true; + } + q.add(proc, false); + } + return ret; + } + // ============================================================================ // Table Queue Lookup Helpers // ============================================================================ diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestMasterProcedureScheduler.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestMasterProcedureScheduler.java index 0cf34126a945..f9ebb9504b4f 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestMasterProcedureScheduler.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestMasterProcedureScheduler.java @@ -1200,4 +1200,28 @@ public void testAcquireSharedLockWhileParentHoldingExclusiveLock() { queue.wakeRegion(proc, regionInfo); queue.wakeTableExclusiveLock(parentProc, tableName); } + + @Test + public void testDirectlyScheduleProcedureThatDoesNotNeedLock() { + TableName tableName = TableName.valueOf(name.getMethodName()); + TestTableProcedure xLockNeededProc = + new TestTableProcedure(1, tableName, TableOperationType.DELETE); + TestTableProcedure noLockNeededProc = + new TestTableProcedure(2, tableName, TableOperationType.READ) { + @Override + public boolean needLock() { + return false; + } + }; + queue.addBack(xLockNeededProc); + queue.addBack(noLockNeededProc); + + assertSame(xLockNeededProc, queue.poll()); + assertEquals(1, queue.size()); + + // now the table exclusive lock has been acquired + assertFalse(queue.waitTableExclusiveLock(xLockNeededProc, tableName)); + + assertSame(noLockNeededProc, queue.poll()); + } } From 847fd0ca0755955ba68cff20d87f1d18a1234bb7 Mon Sep 17 00:00:00 2001 From: huiruan <876107431@qq.com> Date: Sat, 5 Aug 2023 23:28:44 +0800 Subject: [PATCH 2/3] move no lock needed check out of removeFromRunQueue --- .../hadoop/hbase/procedure2/Procedure.java | 7 ++ .../procedure/MasterProcedureScheduler.java | 75 ++++++++++++++----- 2 files changed, 62 insertions(+), 20 deletions(-) diff --git a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java index 361bf6f1d838..8dcf673fecfb 100644 --- a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java +++ b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java @@ -272,6 +272,13 @@ protected boolean waitInitialized(TEnvironment env) { return false; } + /** + * If the procedure does not require any lock protection during execution, you can declare + * that the procedure does not require locks through the following method, which will help + * speed up the scheduling of the procedure. It's a little dangerous to override this method, + * do not change its behavior unless you know what you are doing. See HBASE-27905 for details. + * @return true if procedure requires lock protection during execution, otherwise false + */ public boolean needLock() { return true; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureScheduler.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureScheduler.java index 69cc2011ab91..65f7ca1599b2 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureScheduler.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureScheduler.java @@ -235,8 +235,15 @@ private > Procedure doPoll(final FairQueue fairq) // we are not ready to run, add back and try the next procedure rq.add(proc, false); } - // no procedure is ready for execution, remove from run queue - removeFromRunQueue(fairq, rq, () -> "no procedure can be executed"); + if (hasNoLockNeededProcedure(rq)) { + if (LOG.isTraceEnabled()) { + LOG.trace("DO NOT remove {} from run queue because there are still procedures that do " + + "not need to acquire locks in the queue", rq); + } + } else { + // no procedure is ready for execution, remove from run queue + removeFromRunQueue(fairq, rq, () -> "no procedure can be executed"); + } return null; } @@ -374,13 +381,6 @@ private static > void addToRunQueue(FairQueue fairq, private static > void removeFromRunQueue(FairQueue fairq, Queue queue, Supplier reason) { - if (hasNoLockNeededProcedure(queue)) { - if (LOG.isTraceEnabled()) { - LOG.trace("DO NOT remove {} from run queue because There are still procedures in the " - + "queue that do not need to acquire locks", queue); - } - return; - } if (LOG.isTraceEnabled()) { LOG.trace("Remove {} from run queue because: {}", queue, reason.get()); } @@ -642,8 +642,15 @@ public boolean waitTableExclusiveLock(final Procedure procedure, final TableN logLockedResource(LockedResourceType.TABLE, table.getNameAsString()); return true; } - removeFromRunQueue(tableRunQueue, getTableQueue(table), - () -> procedure + " held the exclusive lock"); + TableQueue queue = getTableQueue(table); + if (hasNoLockNeededProcedure(queue)) { + if (LOG.isTraceEnabled()) { + LOG.trace("DO NOT remove {} from run queue because there are still procedures that do " + + "not need to acquire locks in the queue", queue); + } + } else { + removeFromRunQueue(tableRunQueue, queue, () -> procedure + " held the exclusive lock"); + } return false; } finally { schedUnlock(); @@ -958,12 +965,18 @@ public boolean waitServerExclusiveLock(final Procedure procedure, if (lock.tryExclusiveLock(procedure)) { // In tests we may pass procedures other than ServerProcedureInterface, just pass null if // so. - removeFromRunQueue(serverRunQueue, - getServerQueue(serverName, + ServerQueue queue = getServerQueue(serverName, procedure instanceof ServerProcedureInterface ? (ServerProcedureInterface) procedure - : null), - () -> procedure + " held exclusive lock"); + : null); + if (hasNoLockNeededProcedure(queue)) { + if (LOG.isTraceEnabled()) { + LOG.trace("DO NOT remove {} from run queue because there are still procedures that do " + + "not need to acquire locks in the queue", queue); + } + } else { + removeFromRunQueue(serverRunQueue, queue, () -> procedure + " held exclusive lock"); + } return false; } waitProcedure(lock, procedure); @@ -1016,8 +1029,15 @@ public boolean waitPeerExclusiveLock(Procedure procedure, String peerId) { try { final LockAndQueue lock = locking.getPeerLock(peerId); if (lock.tryExclusiveLock(procedure)) { - removeFromRunQueue(peerRunQueue, getPeerQueue(peerId), - () -> procedure + " held exclusive lock"); + PeerQueue queue = getPeerQueue(peerId); + if (hasNoLockNeededProcedure(queue)) { + if (LOG.isTraceEnabled()) { + LOG.trace("DO NOT remove {} from run queue because there are still procedures that do " + + "not need to acquire locks in the queue", queue); + } + } else { + removeFromRunQueue(peerRunQueue, queue, () -> procedure + " held exclusive lock"); + } return false; } waitProcedure(lock, procedure); @@ -1066,7 +1086,15 @@ public boolean waitMetaExclusiveLock(Procedure procedure) { try { final LockAndQueue lock = locking.getMetaLock(); if (lock.tryExclusiveLock(procedure)) { - removeFromRunQueue(metaRunQueue, getMetaQueue(), () -> procedure + " held exclusive lock"); + MetaQueue queue = getMetaQueue(); + if (hasNoLockNeededProcedure(queue)) { + if (LOG.isTraceEnabled()) { + LOG.trace("DO NOT remove {} from run queue because there are still procedures that do " + + "not need to acquire locks in the queue", queue); + } + } else { + removeFromRunQueue(metaRunQueue, queue, () -> procedure + " held exclusive lock"); + } return false; } waitProcedure(lock, procedure); @@ -1112,8 +1140,15 @@ public boolean waitGlobalExclusiveLock(Procedure procedure, String globalId) try { final LockAndQueue lock = locking.getGlobalLock(globalId); if (lock.tryExclusiveLock(procedure)) { - removeFromRunQueue(globalRunQueue, getGlobalQueue(globalId), - () -> procedure + " held shared lock"); + GlobalQueue queue = getGlobalQueue(globalId); + if (hasNoLockNeededProcedure(queue)) { + if (LOG.isTraceEnabled()) { + LOG.trace("DO NOT remove {} from run queue because there are still procedures that do " + + "not need to acquire locks in the queue", queue); + } + } else { + removeFromRunQueue(globalRunQueue, queue, () -> procedure + " held shared lock"); + } return false; } waitProcedure(lock, procedure); From 2a1603db1df25a9081611fd36de0d2d625046288 Mon Sep 17 00:00:00 2001 From: huiruan <876107431@qq.com> Date: Mon, 7 Aug 2023 21:13:07 +0800 Subject: [PATCH 3/3] run spotless:apply --- .../org/apache/hadoop/hbase/procedure2/Procedure.java | 8 ++++---- .../hbase/master/procedure/MasterProcedureScheduler.java | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java index 8dcf673fecfb..04975b72b3ae 100644 --- a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java +++ b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java @@ -273,10 +273,10 @@ protected boolean waitInitialized(TEnvironment env) { } /** - * If the procedure does not require any lock protection during execution, you can declare - * that the procedure does not require locks through the following method, which will help - * speed up the scheduling of the procedure. It's a little dangerous to override this method, - * do not change its behavior unless you know what you are doing. See HBASE-27905 for details. + * If the procedure does not require any lock protection during execution, you can declare that + * the procedure does not require locks through the following method, which will help speed up the + * scheduling of the procedure. It's a little dangerous to override this method, do not change its + * behavior unless you know what you are doing. See HBASE-27905 for details. * @return true if procedure requires lock protection during execution, otherwise false */ public boolean needLock() { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureScheduler.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureScheduler.java index 65f7ca1599b2..e3008b5c0e0a 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureScheduler.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureScheduler.java @@ -646,7 +646,7 @@ public boolean waitTableExclusiveLock(final Procedure procedure, final TableN if (hasNoLockNeededProcedure(queue)) { if (LOG.isTraceEnabled()) { LOG.trace("DO NOT remove {} from run queue because there are still procedures that do " - + "not need to acquire locks in the queue", queue); + + "not need to acquire locks in the queue", queue); } } else { removeFromRunQueue(tableRunQueue, queue, () -> procedure + " held the exclusive lock"); @@ -966,8 +966,8 @@ public boolean waitServerExclusiveLock(final Procedure procedure, // In tests we may pass procedures other than ServerProcedureInterface, just pass null if // so. ServerQueue queue = getServerQueue(serverName, - procedure instanceof ServerProcedureInterface - ? (ServerProcedureInterface) procedure + procedure instanceof ServerProcedureInterface + ? (ServerProcedureInterface) procedure : null); if (hasNoLockNeededProcedure(queue)) { if (LOG.isTraceEnabled()) {