ctubbsii commented on code in PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#discussion_r857898699
##########
core/src/main/java/org/apache/accumulo/core/rpc/TTimeoutTransport.java:
##########
@@ -87,6 +84,12 @@ TTransport createInternal(SocketAddress addr, long
timeoutMillis) throws TTransp
socket = openSocket(addr, (int) timeoutMillis);
} catch (IOException e) {
// openSocket handles closing the Socket on error
+ if (e instanceof AsynchronousCloseException) {
+ if (Thread.currentThread().isInterrupted()) {
+ Thread.currentThread().interrupt();
+ throw new UncheckedIOException(e);
+ }
+ }
Review Comment:
I think this strategy is okay, but I don't think we actually want to do this
for other AsynchronousCloseExceptions. I'm not sure what exactly can cause
those, but I imagine if those happen, they would happen because of very special
edge cases we'd probably want to explicitly handle.
For this, I think it's sufficient to just check ClosedByInterruptException.
The above code sets the interrupted state flag before it throws
UncheckedIOException. I don't think that's necessary, since the flag will have
already been set. Even if it happens that it had been cleared, I don't think it
matters, because the intent is to let the UncheckedIOException propagate all
the way up and terminate the thread. It doesn't really matter if the flag is
true or false when the thread terminates this way.
So, more succinctly, I think these blocks can be written as:
```suggestion
if (e instanceof ClosedByInterruptException) {
throw new UncheckedIOException(e);
}
```
--
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]
