[GitHub] [hbase] Reidddddd commented on a change in pull request #2574: HBASE-25212 Optionally abort requests in progress after deciding a region should close
Reidd commented on a change in pull request #2574: URL: https://github.com/apache/hbase/pull/2574#discussion_r513897937 ## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java ## @@ -8730,12 +8868,22 @@ public void startRegionOperation() throws IOException { @Override public void startRegionOperation(Operation op) throws IOException { +boolean isInterruptableOp = false; switch (op) { - case GET: // read operations + case GET: // interruptible read operations case SCAN: +isInterruptableOp = true; Review comment: Only one q left, read operations are interruptible, but i couldn't find new interruption handling in read path. Do we just leave it as it is? (just for confirmation) 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
[GitHub] [hbase] Reidddddd commented on a change in pull request #2574: HBASE-25212 Optionally abort requests in progress after deciding a region should close
Reidd commented on a change in pull request #2574: URL: https://github.com/apache/hbase/pull/2574#discussion_r513154120 ## 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> 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: the new commit LGTM, let's wait QA. 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
[GitHub] [hbase] Reidddddd commented on a change in pull request #2574: HBASE-25212 Optionally abort requests in progress after deciding a region should close
Reidd commented on a change in pull request #2574: URL: https://github.com/apache/hbase/pull/2574#discussion_r512404932 ## 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> 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: Could we only call checkInterrupt() **once** before or after WAL synced, instead of a check after every step. Personally I incline to check after WAL synced. It also makes sense to call before WAL synced if considering the sync issue which occasionally happen. 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