Apache9 commented on a change in pull request #2574:
URL: https://github.com/apache/hbase/pull/2574#discussion_r514222462
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -4569,13 +4665,29 @@ private void doMiniBatchMutate(BatchOperation<?>
batchOp) throws IOException {
// We should record the timestamp only after we have acquired the
rowLock,
// otherwise, newer puts/deletes/increment/append are not guaranteed to
have a newer
// timestamp
+
+ // Check for thread interrupt status in case we have been signaled from
+ // #interruptRegionOperation.
+ checkInterrupt();
+
long now = EnvironmentEdgeManager.currentTime();
batchOp.prepareMiniBatchOperations(miniBatchOp, now, acquiredRowLocks);
// STEP 3. Build WAL edit
+
+ // Check for thread interrupt status in case we have been signaled from
+ // #interruptRegionOperation.
+ checkInterrupt();
+
List<Pair<NonceKey, WALEdit>> walEdits =
batchOp.buildWALEdits(miniBatchOp);
// STEP 4. Append the WALEdits to WAL and sync.
+
+ // Check for thread interrupt status in case we have been signaled from
+ // #interruptRegionOperation. This is the last place we can do it
"safely" before
+ // WAL appends.
+ checkInterrupt();
+
Review comment:
After reading this discussion several times, I think the reason we do
not want to niterrupte a WAL sync is that it may lead to a region server abort?
I would say this is not the case here. I checked the code again, the actual
sync is done in the disruptor thread, in the rpc thread we just block on a
SyncFuture(as Andrew mentioned above), the interruption on the rpc thread will
just lead to an IOException tp client, the actual sync operation will not be
interrupted so we are safe.
So I do not think we need to disable interrupts here?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -8777,23 +8936,36 @@ public void closeRegionOperation(Operation operation)
throws IOException {
if (operation == Operation.SNAPSHOT) {
stores.values().forEach(HStore::postSnapshotOperation);
}
+ regionLockHolders.remove(Thread.currentThread());
lock.readLock().unlock();
if (coprocessorHost != null) {
coprocessorHost.postCloseRegionOperation(operation);
}
}
+ /**
+ * If a handler thread is eligible for interrupt, make it ineligible. Should
be paired
+ * with {{@link #enableInterrupts()}.
+ */
+ protected void disableInterrupts() {
+ regionLockHolders.computeIfPresent(Thread.currentThread(), (t,b) -> false);
+ }
+
+ /**
+ * If a handler thread was made ineligible for interrupt via {{@link
#disableInterrupts()},
+ * make it eligible again. No-op if interrupts are already enabled.
+ */
+ protected void enableInterrupts() {
+ regionLockHolders.computeIfPresent(Thread.currentThread(), (t,b) -> true);
+ }
+
/**
* This method needs to be called before any public call that reads or
* modifies stores in bulk. It has to be called just before a try.
* #closeBulkRegionOperation needs to be called in the try's finally block
* Acquires a writelock and checks if the region is closing or closed.
- * @throws NotServingRegionException when the region is closing or closed
Review comment:
Why remove these comments? They are not the cases for now?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -6588,8 +6677,10 @@ protected RowLock getRowLockInternal(byte[] row, boolean
readLock, final RowLock
success = true;
return result;
} catch (InterruptedException ie) {
- LOG.warn("Thread interrupted waiting for lock on row: {}, in region {}",
rowKey,
- getRegionInfo().getRegionNameAsString());
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Thread interrupted waiting for lock on row: {}, in region
{}", rowKey,
Review comment:
I mean the log message. Waiting on row lock is not the only case now?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
