apurtell commented on a change in pull request #2574:
URL: https://github.com/apache/hbase/pull/2574#discussion_r516166902
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -4565,17 +4663,23 @@ private void doMiniBatchMutate(BatchOperation<?>
batchOp) throws IOException {
lock(this.updatesLock.readLock(), miniBatchOp.getReadyToWriteCount());
locked = true;
+ // From this point until memstore update this operation should not be
interrupted.
+ disableInterrupts();
Review comment:
Yes, here I can put a checkInterrupt. Let me do that.
If we remove the handler from the map at this point, no other mini batch
will be eligible for interrupt.
batchMutate does this:
while (!batchOp.isDone()) {
...
doMiniBatchMutate(batchOp);
...
}
If we remove the handler from the map only the first iteration through the
loop can be interrupted.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -4565,17 +4663,23 @@ private void doMiniBatchMutate(BatchOperation<?>
batchOp) throws IOException {
lock(this.updatesLock.readLock(), miniBatchOp.getReadyToWriteCount());
locked = true;
+ // From this point until memstore update this operation should not be
interrupted.
+ disableInterrupts();
Review comment:
Yes, here I can put a checkInterrupt. Let me do that.
If we remove the handler from the map at this point, no other mini batch
will be eligible for interrupt.
batchMutate does this:
while (!batchOp.isDone()) {
...
doMiniBatchMutate(batchOp);
...
}
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -8303,9 +8428,14 @@ public void processRowsWithLocks(RowProcessor<?,?>
processor, long timeout,
prevRowLock = rowLock;
}
}
+
// STEP 3. Region lock
lock(this.updatesLock.readLock(), acquiredRowLocks.isEmpty() ? 1 :
acquiredRowLocks.size());
locked = true;
+
+ // From this point until memstore update this operation should not be
interrupted.
+ disableInterrupts();
Review comment:
Will add a checkInterrupt here too.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -7623,6 +7740,9 @@ protected boolean nextRow(ScannerContext scannerContext,
Cell curRowCell) throws
Cell next;
while ((next = this.storeHeap.peek()) != null &&
CellUtil.matchingRows(next, curRowCell)) {
+ // Check for thread interrupt status in case we have been signaled from
+ // #interruptRegionOperation.
+ checkInterrupt();
Review comment:
Good point. Agreed it would be a good follow up.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -8303,9 +8428,14 @@ public void processRowsWithLocks(RowProcessor<?,?>
processor, long timeout,
prevRowLock = rowLock;
}
}
+
// STEP 3. Region lock
lock(this.updatesLock.readLock(), acquiredRowLocks.isEmpty() ? 1 :
acquiredRowLocks.size());
locked = true;
+
+ // From this point until memstore update this operation should not be
interrupted.
+ disableInterrupts();
Review comment:
This code is not executed in a loop so removing the thread from the map
would be fine. However the reason why I also do disable/enable interrupt is as
follows: We already have to do this for doMiniBatchMutation. The disable/enable
interrupt pair is a new code discipline. This section of the code also has the
same requirements. Apply the new code discipline here too for consistency.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -8303,9 +8428,14 @@ public void processRowsWithLocks(RowProcessor<?,?>
processor, long timeout,
prevRowLock = rowLock;
}
}
+
// STEP 3. Region lock
lock(this.updatesLock.readLock(), acquiredRowLocks.isEmpty() ? 1 :
acquiredRowLocks.size());
locked = true;
+
+ // From this point until memstore update this operation should not be
interrupted.
+ disableInterrupts();
Review comment:
This code is not executed in a loop so removing the thread from the map
would be fine. However the reason why I also do disable/enable interrupt is as
follows: We already have to do this for doMiniBatchMutation. (The
disable/enable pair in doMiniBatchMutation protects WAL update and memstore
insert from interrupt and makes them "atomic" in this sense, and disable/enable
makes sense there because doMiniBatchMutation is inside a loop.) The
disable/enable interrupt pair is a new code discipline. This section of the
code also has the same requirements. Apply the new code discipline here too for
consistency.
----------------------------------------------------------------
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
