[GitHub] [accumulo] EdColeman commented on a diff in pull request #2762: Update spotbugs

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

EdColeman commented on code in PR #2762:
URL: https://github.com/apache/accumulo/pull/2762#discussion_r892827017


##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ConditionalWriterImpl.java:
##########
@@ -146,7 +146,7 @@ public Result next() {
         count--;
         return result;
       } catch (InterruptedException e) {
-        throw new RuntimeException(e);
+        throw new IllegalStateException(e);

Review Comment:
   Could add 
   ```
   Thread.currentThread().interrupt();
   ```



##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ReplicationOperationsImpl.java:
##########
@@ -97,7 +97,7 @@ public void drain(final String tableName, final Set<String> wals)
           Thread.sleep(1000);
         } catch (InterruptedException e) {
           Thread.currentThread().interrupt();
-          throw new RuntimeException("Thread interrupted", e);
+          throw new IllegalStateException("Thread interrupted", e);

Review Comment:
   Could add 
   ```
   Thread.currentThread().interrupt();
   ```



##########
core/src/main/java/org/apache/accumulo/core/util/compaction/ExternalCompactionUtil.java:
##########
@@ -108,7 +108,7 @@ public static Optional<HostAndPort> findCompactionCoordinator(ClientContext cont
       }
       return Optional.of(HostAndPort.fromString(new String(address)));
     } catch (KeeperException | InterruptedException e) {
-      throw new RuntimeException(e);
+      throw new IllegalStateException(e);

Review Comment:
   Not sure if it matters - but could reassert the interrupt flag on InterruptedException.



##########
core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransport.java:
##########
@@ -56,8 +57,10 @@ public void open() throws TTransportException {
         }
         return null;
       });
-    } catch (IOException | InterruptedException e) {
-      throw new RuntimeException(e);
+    } catch (IOException e) {
+      throw new UncheckedIOException(e);
+    } catch (InterruptedException e) {
+      throw new IllegalStateException(e);

Review Comment:
   Could add:
   ```
   Thread.currentThread().interrupt();
   ```
   



##########
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/lru/LruBlockCache.java:
##########
@@ -155,7 +155,7 @@ public LruBlockCache(final LruBlockCacheConfiguration conf) {
         try {
           Thread.sleep(10);
         } catch (InterruptedException ex) {
-          throw new RuntimeException(ex);
+          throw new IllegalStateException(ex);

Review Comment:
   Could add: 
   ```
   Thread.currentThread().interrupt();
   ```



##########
core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java:
##########
@@ -85,27 +85,24 @@ public static final ThreadPools getClientThreadPools(UncaughtExceptionHandler ue
   private static final ConcurrentLinkedQueue<ScheduledFuture<?>> NON_CRITICAL_RUNNING_TASKS =
       new ConcurrentLinkedQueue<>();
 
-  private static Runnable TASK_CHECKER = new Runnable() {
-    @Override
-    public void run() {
-      final List<ConcurrentLinkedQueue<ScheduledFuture<?>>> queues =
-          List.of(CRITICAL_RUNNING_TASKS, NON_CRITICAL_RUNNING_TASKS);
-      while (true) {
-        queues.forEach(q -> {
-          Iterator<ScheduledFuture<?>> tasks = q.iterator();
-          while (tasks.hasNext()) {
-            if (checkTaskFailed(tasks.next(), q)) {
-              tasks.remove();
-            }
+  private static Runnable TASK_CHECKER = () -> {
+    final List<ConcurrentLinkedQueue<ScheduledFuture<?>>> queues =
+        List.of(CRITICAL_RUNNING_TASKS, NON_CRITICAL_RUNNING_TASKS);
+    while (true) {
+      queues.forEach(q -> {
+        Iterator<ScheduledFuture<?>> tasks = q.iterator();
+        while (tasks.hasNext()) {
+          if (checkTaskFailed(tasks.next(), q)) {
+            tasks.remove();
           }
-        });
-        try {
-          TimeUnit.MINUTES.sleep(1);
-        } catch (InterruptedException ie) {
-          // This thread was interrupted by something while sleeping. We don't want to exit
-          // this thread, so reset the interrupt state on this thread and keep going.
-          Thread.interrupted();
         }
+      });
+      try {
+        TimeUnit.MINUTES.sleep(1);
+      } catch (InterruptedException ie) {
+        // This thread was interrupted by something while sleeping. We don't want to exit
+        // this thread, so reset the interrupt state on this thread and keep going.
+        Thread.interrupted();

Review Comment:
   Should this be `Thread.currentThread().interrupt();`



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