virajjasani commented on code in PR #6789:
URL: https://github.com/apache/hbase/pull/6789#discussion_r2032131584
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:
##########
@@ -8441,9 +8446,9 @@ public void startRegionOperation(Operation op) throws
IOException {
// Update regionLockHolders ONLY for any startRegionOperation call that is
invoked from
// an RPC handler
Thread thisThread = Thread.currentThread();
- if (isInterruptableOp) {
Review Comment:
`disableInterrupts()` and `enableInterrupts()` are only meant for the
mutation RPCs: interruption is disabled right before making WAL edit until the
batchMutation is completed. Hence, only mutation RPCs will go through this
cycle.
Hence, as far as other non-interruptible threads are concerned, even if they
do stay in the same map (like earlier version), we are not going to enable
interruption for them. Similarly, any non-mutation interruptible threads are
not going to be converted to non-interruptible by any workflow. So, single map
was sufficient IMO.
I agree with David above that the comment should reflect the new code
changes. But having two maps is becoming bit difficult to understand, more so
when one looks back at the history of which change had to introduce two maps,
it might get even more difficult to understand the before and after patch
updates :)
Semantic changes in the new version of the PR are bit difficult to grasp
(e.g. interruptible threads are on separate map but still they can be made
non-interruptible while there is separate map for non-interruptible threads).
--
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]
