kevinrr888 commented on code in PR #5613:
URL: https://github.com/apache/accumulo/pull/5613#discussion_r2130245995
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java:
##########
@@ -94,17 +104,20 @@ private ThriftTransportPool(LongSupplier maxAgeMillis) {
} catch (TransportPoolShutdownException e) {
log.debug("Error closing expired connections", e);
}
- });
+ };
Review Comment:
> With this change the server will terminate if this thread exits for any
reason
The server will only terminate if the thread exits due to an uncaught
`RuntimeException` (handled in the `createCriticalThread` method) or if an
uncaught `Error` occurs (handled in the `AccumuloUncaughtExceptionHandler`). If
the thread exists normally, the server will not be terminated.
> The AccumuloUncaughtExceptionHandler will be invoked for any uncaught
exception that causes this thread to exit (...) The logging in
AccumuloUncaughtExceptionHandler should be sufficient for other
RuntimeException's.
That just logs that the thread is dead in the case of `RuntimeException`s.
The point of the new `createCriticalThread` method was to allow all unchecked
exceptions to be handled (all `Error`s kill the server via
`AccumuloUncaughtExceptionHandler` and we choose whether a `RuntimeException`
will kill the server via `createCriticalThread` or `createNonCriticalThread`).
The reason for `createCriticalThread` was that logging is probably not
sufficient in the case of `RuntimeException`s for some threads (e.g., if the
thread is created only once and not tracked/restarted on failures, the server
is missing important functionality and should halt now)
> Because InterruptedException is caught, the UEH will not be invoked, and
nothing will be logged. We likely want to add logging in the
InterruptedException case
Yeah, I agree logging could be added here.
--
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]
