Update to the latest log4j and spotbugs versions - #6486
Conversation
Bumps versions and fixes deprecated builder pattern
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
The build is still failing. I think it's probably okay to suppress these in 2.1, but in main, we should work to remove the problems spotbugs found, because we do a lot less static stuff in the main branch now, and we can probably do better with regards to these checks there.
Added private locks for methods that were synchronized to fix spotbugs findings
Runnable requires a public run() method so these can't be fixed
Fixed these spotbug findings
The intent of this code is not clear. This may not be the correct way to do this but will review it at the PR stage.
dlmarion
left a comment
dlmarion
left a comment
There was a problem hiding this comment.
I'm a little confused why we are making so many changes if we are also excluding these spotbugs issues.
| ThreadPools.watchCriticalScheduledTask(context.getScheduledExecutor().scheduleWithFixedDelay( | ||
| new CompactionWatcher(context.getConfiguration()), 10000, 10000, TimeUnit.MILLISECONDS)); | ||
| watching = true; | ||
| public static void startWatching(ServerContext context) { |
There was a problem hiding this comment.
Does the exclusion not cover this case? Just curious why we are fixing some of these but also have the exclusion.
| // remove old settings from zookeeper first, if any | ||
| ZooReaderWriter zoo = context.getZooReaderWriter(); | ||
| synchronized (zooCache) { | ||
| synchronized (zooCacheLock) { |
There was a problem hiding this comment.
Why did this have to change?
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
These suppressions are fine for the 2.1 branch, but I think working through these in the main branch so we can move towards a better concurrency model that's easier for QA tooling to verify would be a great goal.
I think the DistributedWorkQueue logic is broken, and some of the annotated inline suppressions need a justification parameter.
| } | ||
|
|
||
| @Override | ||
| @SuppressFBWarnings("USO_UNSAFE_OBJECT_SYNCHRONIZATION") |
There was a problem hiding this comment.
Annotated spotbugs suppressions should include a justification
| import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; | ||
|
|
||
| @SuppressFBWarnings(value = "NP_NULL_PARAM_DEREF_NONVIRTUAL", | ||
| justification = "Test code is testing that a NPE is thrown") |
There was a problem hiding this comment.
This false positive for assertThrows is probably something that spotbugs should just ignore automatically. Could open a bug report upstream to indicate that these are false positives.
| private final Object condVar = new Object(); | ||
|
|
||
| final Object condVar = new Object(); | ||
| public void waitUntilDone(Set<String> workIDs) throws KeeperException, InterruptedException { |
There was a problem hiding this comment.
I don't think this is correct. This would create a global lock object for all calls to waitUntilDone with different IDs. However, this appears to require a separate one for each call to this method.
I'm not sure the notify/wait logic is correct either (potential preexisting problem), since it looks like the synchronization prevents the object from being notified while it is waiting. It looks like it can only notify once the 10 second wait is done... which defeats the point, because it doesn't need to be notified at that point, since it's already back to iterating in the loop.
A proper fix might remove the NN_NAKED_NOTIFY warning suppression as well.
3 participants
Bumps versions and fixes deprecated builder pattern