nikita-sirohi commented on PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#issuecomment-1095817099

   Thanks for the feedback & speedy response. You are correct -- normally we 
would see an InterruptedException get thrown. 
   
   In this case, though, what is actually thrown is an IOException in the form 
of ClosedByInterruptException when trying to open a socket: 
https://docs.oracle.com/javase/9/docs/api/java/nio/channels/ClosedByInterruptException.html
 . 
   
   "Checked exception received by a thread when another thread interrupts it 
while it is blocked in an I/O operation upon a channel. Before this exception 
is thrown the channel will have been closed and the interrupt status of the 
previously-blocked thread will have been set."
   
   This ends up being swallowed and logged, so we see an endless loop of "WARN 
ThriftUtil: Failed to open transport..."
   
   If you look in ThriftUtil.java, createClientTransport:
   
   I see this code:
   
   `      try {
             transport = TTimeoutTransport.create(address, timeout);
           } catch (TTransportException e) {
             log.warn("Failed to open transport to {}", address);
             throw e;
           }`
   
   TTimeoutTransport.create ends up here:
   
   `  TTransport createInternal(SocketAddress addr, long timeoutMillis) throws 
TTransportException {
       Socket socket = null;
       try {
         socket = openSocket(addr, (int) timeoutMillis);
       } catch (IOException e) {
         // openSocket handles closing the Socket on error
         throw new TTransportException(e);
       }`
   
   Since this manifests as an IOException, it's turned into a 
TTransportException. Gets logged and re thrown, then in the execute loop, 
retried forever.
   
   I made the patch handle in this manner because this was the pattern I saw 
elsewhere -- in the ThriftServer while loop. So I thought this was the 
appropriate pattern. I also wasn't sure if either createClientTransport or 
TTimeoutTransport.create had the appropriate information to special case this 
exception -- felt like the caller should be handling it. More than happy to 
move this / handle differently though? Let me know.


-- 
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]

Reply via email to