dlmarion commented on PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#issuecomment-1100081640
I'm thinking that this patch addresses only one case, but there could be
other places where this might happen. For example, we have the same problem in
`ManagerClient.execute`, `ManagerClient.executeGeneric`, and
`ManagerClient.cancelFateOperation`. There are multiple places in ThriftUtil
where the log message `Failed to open transport to {}` is logged. Two of those
places are when calling `TTimeoutTransport.create` and the other two are when
setting up `TSaslClientTransport`.
For the `TTimeoutTransport.create` case, I wonder if the right solution is
to catch `ClosedByInterruptException` and throw a new `InterruptedException`.
The Threads' `interrupted` state is already set when the
`ClosedByInterruptException` is raised by the underlying Socket code. I think
that `ThriftUtil.createClientTransport` should *not* handle the
`InterruptedException` and should let it be handled by the caller. In the
`ServerClient` and `ManagerClient` cases, the `InterruptedException` should be
handled outside of the `while` loop and `Thread.interrupted` should be called
to reset the Threads' interrupt state.
For the `TSaslClientTransport` case, I don't think we have an opportunity to
catch `ClosedByInterruptException`. Instead we would have to see if that might
be the cause of one of the exceptions that it raises and then throw a new
InterruptedException.
--
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]
