jiacheliu3 commented on PR #733: URL: https://github.com/apache/ratis/pull/733#issuecomment-1248970630
> @jiacheliu3 , thanks for the update! After checked the Java API in more details, it seems better to pass a `ThreadGroup` instead a `UncaughtExceptionHandler` (sorry that it was a bad idea); see https://issues.apache.org/jira/secure/attachment/13049347/733_review.patch @szetszwo Thanks for the suggestion that makes sense to me. One subtle implication I'm not sure about is, if the `ThreadGroup` is left `null` when creating a thread, that thread uses the current thread's `ThreadGroup`: [code](https://docs.oracle.com/javase/8/docs/api/java/lang/Thread.html#Thread-java.lang.ThreadGroup-java.lang.Runnable-) I'm not 100% sure what that suggests. Specifically I'm not sure if threads from `TimeoutScheduler.newExecutor()` [code](https://github.com/apache/ratis/blob/8fbe434efad64436e5505f5a03a4ef048a425a0a/ratis-common/src/main/java/org/apache/ratis/util/TimeoutScheduler.java#L89) will implicitly also inherit this `ThreadGroup` and uncaught exceptions there will propagate to the application in the same way. **Is that true** and is that desirable? In other words I imagine there are critical threads whose uncaught exceptions should crash the `RaftServer`, whereas there are unimportant threads whose crashes should be somehow tolerated and just generate a warning at most. In contrast if we specify an `UncaughtExceptionHandler` in the old way, it is possible to distinguish the critical threads from the others. Not saying `setUncaughtExceptionHandler()` doesn't have downsides, just saying it's easier to manually configure. -- 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]
