nikita-sirohi commented on PR #2622: URL: https://github.com/apache/accumulo/pull/2622#issuecomment-1101042541
Thanks everyone for the feedback. Want to make sure I understand the salient points here: * If we go with the "Thread.currentThread().isInterrupted()" solution, we should reset the interrupted state -- this makes 100% sense to me, can/will fix. * Need to move the exception to the try/catch so that the finally executes (and add a bit more detail to the exception) -- again, makes total sense if we go with the while loop soln * This patch addresses only one case, but there could be other places where this might happen. Makes sense. Wanted to make sure I understand a few points though @dlmarion : > I think that ThriftUtil.createClientTransport should _not_ handle the InterruptedException and should let it be handled by the caller. That was my inclination as well. > 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. 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? > For the TTimeoutTransport.create case, I wonder if the right solution is to catch ClosedByInterruptException in TTimeoutTransport.openSocket and throw a new InterruptedException. 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. We're essentially patching over that behavior. I don't understand well enough the reasoning behind the original behavior to know if this is a bad idea for some reason. > 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. That makes sense -- could just check for (and then reset) thread state interrupted the way I did in the original PR. -- 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]
