keith-turner commented on pull request #2340: URL: https://github.com/apache/accumulo/pull/2340#issuecomment-959342501
>Regarding the BulkImport code above, I believe that in this case an Error would be handled by the AccumuloUncaughtExceptionHandler as this catches Exception, not Error, and the executor that is being used to run the CompletableFuture is being created by ThreadPools. When dealing with futures in Java, Java will take your runnable, supplier,caller, etc and wrap it with one of the following before queueing it on the executor. That code catches throwable before the executor can ever see it. https://github.com/openjdk/jdk/blob/jdk-11%2B28/src/java.base/share/classes/java/util/concurrent/CompletableFuture.java#L1683-L1708 https://github.com/openjdk/jdk/blob/da75f3c4ad5bdf25167a3ed80e51f567ab3dbd01/src/java.base/share/classes/java/util/concurrent/FutureTask.java#L263-L270 >> If the goal of this change is to make it so that Accumulo client code never has to catch Error > > This change is to implement your suggestion in #2331 where you said: > >> I am not sure its appropriate for this particular case but one thing to consider in general from an API perspective is letting >> users pass in ExecutorServices and Thread Factories. Yeah I realize that and I was uncertain about the potential solution then and I am still uncertain about it. This PR has helped me think about it more deeply and see nuances that I was not aware of before. Like before this PR I never realized how complex the ThreadPoolCreation process was and I thought a thread pool factory might be a simple solution to the problem. Also while looking at this PR, I was thinking about instrumentation vs creation. Here is the instrumentation idea that I thought of and am also very uncertain about wether its a good solution. Instead of thread pool factory we could create thread pools in Accumulo client code and provide a user mechanism to instrument those thread pools like the following. ```java interface AccumuloClientInstrumentation { ThreadPoolExecutor instrument(ThreadPoolExecutor tpe); ScheduledThreadPoolExecutor instrument(ScheduledThreadPoolExecutor tpe); } ``` ```java class AccumuloClient { /** * When the following is set, Accumulo will call the instrumentation object whenever it creates any object that can be instrumented by the interface. */ void enableInstrumentation(AccumuloClientInstrumentation aci); } ``` The thing I don't like about this is that it seems like it could break accumulo, maybe that is ok though. Also seems like it could be tricky to use correctly like the thread pool factory concept. I would not know if its workable/good w/o trying it like this PR is trying to thread pool factories. -- 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]
