You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/06/08 20:01:38 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request, #2762: Update spotbugs
ctubbsii opened a new pull request, #2762:
URL: https://github.com/apache/accumulo/pull/2762
This PR updates spotbugs-maven-plugin and adresses all the new warnings
--
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
[GitHub] [accumulo] EdColeman commented on a diff in pull request #2762: Update spotbugs
Posted by 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
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2762: Update spotbugs
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2762:
URL: https://github.com/apache/accumulo/pull/2762#discussion_r893603429
##########
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:
I was avoiding changing behavior in this PR. Those can be added as a separate task.
--
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
[GitHub] [accumulo] ctubbsii closed pull request #2762: Update spotbugs
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii closed pull request #2762: Update spotbugs
URL: https://github.com/apache/accumulo/pull/2762
--
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
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2762: Update spotbugs
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2762:
URL: https://github.com/apache/accumulo/pull/2762#discussion_r893605866
##########
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:
I'm not sure why this comment says "We don't want to exit this thread". I'm not sure why we wouldn't want to exit the thread. In any case, this PR is not intended to change behavior. If that code needs to change to handle the InterruptedException better, then that can be done as a separate task.
--
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
[GitHub] [accumulo] ctubbsii commented on pull request #2762: Update spotbugs
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #2762:
URL: https://github.com/apache/accumulo/pull/2762#issuecomment-1549854286
This PR was originally done to try to update to spotbugs 4.7.0.0, which was very aggressive about exception handling. We are now using 4.7.3.4, which backed off on some of those aggressive checks. However, there are still some improvements that can be made based on my original PR that I was working on. So, I'll close this and those changes will be provided in a new PR, #3402
--
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