dlmarion commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1116937269
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ConditionalWriterImpl.java:
##########
@@ -679,7 +679,7 @@ private void invalidateSession(SessionID sessionId,
HostAndPort location)
LockID lid = new LockID(context.getZooKeeperRoot() + Constants.ZTSERVERS,
sessionId.lockId);
- while (true) {
+ while (true && !Thread.currentThread().isInterrupted()) {
Review Comment:
I think that prior to this change this loop would exit either when
invalidation was successful or if an error was thrown. I think this introduces
a new condition where the loop exits but it's not successful nor is an error
thrown. I'm assuming here (because I didn't look) that the calling code will
assume success when this returns, even though it may not have succeeded. This
would be a good place to throw InterruptedException if we want to handle it.
##########
core/src/main/java/org/apache/accumulo/core/client/IsolatedScanner.java:
##########
@@ -114,7 +114,8 @@ private void readRow() {
}
// wait a moment before retrying
- sleepUninterruptibly(100, MILLISECONDS);
+ // ignore interrupt status
+ UtilWaitThread.sleep(100, MILLISECONDS);
Review Comment:
There are a lot of places in this PR where we are using this new method but
intentionally ignoring the return value and the thread interrupt status. For
example:
```
// ignore interrupt status
UtilWaitThread.sleep(100, MILLISECONDS);
```
I think this may be incorrect. If we are really going to ignore the thread
interrupt status, then we should be resetting it back to `false` if it is
`true`. I think, in the cases like this where we are ignoring the interrupt and
return value, calling `Uninterruptibles.sleepUninterruptibly` is the right
thing to do.
--
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]
