dlmarion commented on PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#issuecomment-1101339181
> Re-calling Thread.interrupted makes sense to me, but why handle this
outside of the while loop as opposed to in the try/catch? Is the thought here
surrounding the loops in a try/catch that checks for an InterruptedException
and converts it to an AccumuloException? Or at that point, why check for the
exception at all?
I guess I was looking at how TTransportException was handled (logged) and it
didn't break out of the loop. As long as you break out of the loop somehow I
think that is sufficient.
> This seems like a good option; my only concern here would be that the
original exception is explicitly thrown as an IOException and the
InterruptedException is not thrown.
It seems to me that `ClosedByInterruptException` is both an IOException and
an InterruptedException as the reason it's thrown is the IO thread is
interrupted by another thread, and then the interrupted state is set on the IO
thread. I suggested throwing an InterruptedException in this case so that
callers would know to handle the interrupted state appropriately.
> could just check for (and then reset) thread state interrupted the way I
did in the original PR
You could do that, but there may be a case where the parent class to
`ClosedByInterruptException` is thrown ( I don't actually know if this is
true). `AsynchronousCloseException` is the parent, has a similar description,
but does not describe setting the interrupt state.
I went back and read what @ctubbsii wrote:
> Another alternative to consider: everywhere we throw new
TTransportException because of an IOException, we could check the IOException
is an instanceof ClosedByInterruptException and instead of wrapping it with
TTransportException, we throw UncheckedIOException and throw that instead.
There's only a few such occurrences in our code.
If we changed it to the following, then I think we would handle more cases
but be less intrusive:
Another alternative to consider: everywhere we throw new TTransportException
in `ThriftUtil.createClientTransport` because of an IOException, we could:
1. check the IOException is an instanceof `AsynchronousCloseException`
2. check if the Thread is interrupted and if so reset the interrupt state
3. and instead of wrapping it with TTransportException, we throw
UncheckedIOException instead.
--
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]
