[GitHub] [accumulo] dlmarion commented on pull request #2622: Handle thread interrupt in server client execute loop.

[GitBox <gi...@apache.org>]

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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org