ichsansaid commented on code in PR #8566:
URL: https://github.com/apache/hbase/pull/8566#discussion_r3968656885
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:
##########
@@ -1257,18 +1263,155 @@ private void parallelSeek(final List<? extends
KeyValueScanner> scanners, final
latch.countDown();
}
}
+ InterruptedIOException interruptedException = null;
+ while(true) {
+ try {
+ latch.await();
+ break;
+ } catch (InterruptedException ie) {
+ interruptedException = (InterruptedIOException) new
InterruptedIOException().initCause(ie);
+ }
+ }
+ if (interruptedException != null) {
Review Comment:
Good catch. I've added Thread.currentThread().interrupt() to restore the
interrupt status, consistent with the adaptive path.
That said, I'd like to open a discussion on the broader behavior. With the
new wait-for-handler approach, an interrupted parallelSeek now blocks until all
background handlers finish before throwing. This raises a concern around
cancellation latency, I'm not certain what triggers the interrupt in practice
(client cancellation, RPC timeout, region close?), but if it is used as a stop
signal, waiting for handlers doing disk/HDFS I/O could delay the response
noticeably. Could you clarify what the typical interrupt sources are here?
The motivation for waiting is resource safety, throwing immediately while
handlers still hold references to the scanner risks use-after-free or resource
leaks if the caller closes the scanner after catching the exception.
Worth noting that this concern already exists in adaptiveParallelSeek, which
has the same wait-for-latch pattern. So a proper fix (e.g. interrupting handler
threads before waiting, or adding a bounded timeout) would need to be applied
to both paths consistently.
A possible middle ground would be to interrupt the handler threads before
waiting on the latch, so they can bail out quickly, or add a bounded timeout to
the wait. Alternatively, as you suggested, we could leave parallelSeek behavior
unchanged and handle this in a separate JIRA, which might actually be the
cleaner option since any fix should cover both paths.
Happy to go with whichever direction you prefer.
--
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]
