[GitHub] [accumulo] ctubbsii commented on a change in pull request #2554: Allow UncaughtExceptionHandler to be overridden in client

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #2554:
URL: https://github.com/apache/accumulo/pull/2554#discussion_r822265748



##########
File path: core/src/main/java/org/apache/accumulo/core/client/AccumuloClient.java
##########
@@ -346,6 +347,16 @@ ConditionalWriter createConditionalWriter(String tableName, ConditionalWriterCon
    */
   interface ClientFactory<T> {
 
+    /**
+     * Override default handling of uncaught exceptions in client threads
+     *
+     * @param ueh
+     *          UncaughtExceptionHandler implementation
+     * @return AccumuloClient or Properties
+     * @since 2.1.0
+     */
+    ClientFactory<T> withUncaughtExceptionHandler(Class<? extends UncaughtExceptionHandler> ueh);

Review comment:
       Instead of providing a class that we must instantiate (and may not have the expected constructors), why not require the instance of the class?
   
   ```suggestion
       ClientFactory<T> withUncaughtExceptionHandler(UncaughtExceptionHandler ueh);
   ```

##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java
##########
@@ -164,6 +168,19 @@ public ClientContext(SingletonReservation reservation, ClientInfo info,
     this.singletonReservation = Objects.requireNonNull(reservation);
     this.tableops = new TableOperationsImpl(this);
     this.namespaceops = new NamespaceOperationsImpl(this, tableops);
+    String uehClassName = serverConf.get(ClientProperty.UNCAUGHT_EXCEPTION_HANDLER.getKey());
+    if (!StringUtils.isEmpty(uehClassName)) {
+      try {
+        @SuppressWarnings("unchecked")
+        Class<? extends UncaughtExceptionHandler> clazz =
+            (Class<? extends UncaughtExceptionHandler>) Class.forName(uehClassName);
+        Threads.setUncaughtExceptionHandler(clazz.getDeclaredConstructor().newInstance());
+      } catch (ClassNotFoundException | InstantiationException | IllegalAccessException
+          | IllegalArgumentException | InvocationTargetException | NoSuchMethodException
+          | SecurityException e) {

Review comment:
       Several of these extend `ReflectiveOperationException`. Catching that should replace several others.

##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java
##########
@@ -164,6 +168,19 @@ public ClientContext(SingletonReservation reservation, ClientInfo info,
     this.singletonReservation = Objects.requireNonNull(reservation);
     this.tableops = new TableOperationsImpl(this);
     this.namespaceops = new NamespaceOperationsImpl(this, tableops);
+    String uehClassName = serverConf.get(ClientProperty.UNCAUGHT_EXCEPTION_HANDLER.getKey());
+    if (!StringUtils.isEmpty(uehClassName)) {
+      try {
+        @SuppressWarnings("unchecked")
+        Class<? extends UncaughtExceptionHandler> clazz =
+            (Class<? extends UncaughtExceptionHandler>) Class.forName(uehClassName);

Review comment:
       ```suggestion
           var clazz = Class.forName(uehClassName).asSubclass(UncaughtExceptionHandler.class);
   ```

##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java
##########
@@ -164,6 +168,19 @@ public ClientContext(SingletonReservation reservation, ClientInfo info,
     this.singletonReservation = Objects.requireNonNull(reservation);
     this.tableops = new TableOperationsImpl(this);
     this.namespaceops = new NamespaceOperationsImpl(this, tableops);
+    String uehClassName = serverConf.get(ClientProperty.UNCAUGHT_EXCEPTION_HANDLER.getKey());
+    if (!StringUtils.isEmpty(uehClassName)) {
+      try {
+        @SuppressWarnings("unchecked")
+        Class<? extends UncaughtExceptionHandler> clazz =
+            (Class<? extends UncaughtExceptionHandler>) Class.forName(uehClassName);
+        Threads.setUncaughtExceptionHandler(clazz.getDeclaredConstructor().newInstance());
+      } catch (ClassNotFoundException | InstantiationException | IllegalAccessException
+          | IllegalArgumentException | InvocationTargetException | NoSuchMethodException
+          | SecurityException e) {
+        throw new RuntimeException("Error setting uncaughtExceptionHandler", e);

Review comment:
       Could choose a more specific RTE. IAE seems appropriate, since it is instantiated by a value provided by the user. It's a bit late to be throwing that, since it's not directly in the builder, but it's probably still better than ISE. Could throw this earlier, in the builder.

##########
File path: core/src/main/java/org/apache/accumulo/core/util/threads/Threads.java
##########
@@ -25,6 +25,8 @@
 
 public class Threads {
 
+  private static UncaughtExceptionHandler UEH = new AccumuloUncaughtExceptionHandler();

Review comment:
       We should be taking care to avoid using JVM-wide static state. What if the user creates two clients with different exception handlers in the same JVM?




-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org