dlmarion commented on PR #5796:
URL: https://github.com/apache/accumulo/pull/5796#issuecomment-3184619057

   > If an exception is thrown in the `.runServer()` methods, then `.close()` 
is never called. Does that cause any issues?
   > 
   
   I don't think so. ServerContext.close does call close on the shared 
conditional writers for the user and metadata tables, but I think if an 
exception is thrown from server's run method, then we want to exit as quickly 
as possible.
   
   > > The above was never logged, instead the only thing that I saw in the log 
was the fact that the server's lock was gone and that the JVM was being halted.
   > 
   > Is this due to the ZK lock being lost by closing the client context? I'm 
curious that one logger is able to print a message showing the lock is gone, 
but the exception message wasn't flushed.
   
   It's due to close being called and the JVM halting due to the lock watcher 
firing before the code in `Main.die` executes.
   
   > 
   > I thought try-with-resources will append any exception in the `close()` 
method with the one from the original resource. I wonder if it "holding onto" 
the original exception message is why it's not ending up in the error log.
   > 
   > In general, if we don't care about any cleanup actions being called in the 
`.close()` then this change seems fine, but I'm not 100% sure it solves the 
problem and without code comments, I could see this change getting accidentally 
refactored back into try-with resource blocks.
   > 
   > Could the exception be logged in `AbstractServer.runServer()` before 
throwing the exception up to the try-with-resources block in the various Main 
methods?
   > 
   > ```
   >     Throwable thrown = err.get();
   >     if (thrown != null) {
   >       log.error(thrown.getMessage());
   >       if (thrown instanceof Error) {
   >         throw (Error) thrown;
   >       }
   >       if (thrown instanceof Exception) {
   >         throw (Exception) thrown;
   >       }
   >       throw new RuntimeException("Weird throwable type thrown", thrown);
   >     }
   > ```
   
   Yes, we could do that. I did do that and I saw the logging show up. The 
place where the logging is done currently is in `Main.die`, called from 
`Main.execKeyword`.
   


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