ctubbsii commented on code in PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#discussion_r2271795563
##########
core/src/main/java/org/apache/accumulo/core/fate/Fate.java:
##########
@@ -572,4 +572,10 @@ private void waitForDeadResCleanerShutdown(long start,
long timeout, TimeUnit ti
}
}
}
+
+ @Override
+ @SuppressWarnings("deprecation")
+ public final void finalize() {
+ // Prevent finalizer attacks (SpotBugs CT_CONSTRUCTOR_THROW)
+ }
Review Comment:
I'd really rather not add these methods in so many places. If this isn't
possible to address another way, I think we can just suppress it. Adding this
deprecated method is probably the least good workaround.
##########
assemble/pom.xml:
##########
@@ -527,6 +527,13 @@
</plugins>
</pluginManagement>
<plugins>
+ <plugin>
+ <groupId>com.github.spotbugs</groupId>
+ <artifactId>spotbugs-maven-plugin</artifactId>
+ <configuration>
+
<omitVisitors>SharedVariableAtomicityDetector,ConstructorThrow</omitVisitors>
+ </configuration>
+ </plugin>
Review Comment:
This doesn't look right. There's no code in this module. This config
probably doesn't need to be in this module.
##########
core/pom.xml:
##########
@@ -242,6 +242,13 @@
</plugins>
</pluginManagement>
<plugins>
+ <plugin>
+ <groupId>com.github.spotbugs</groupId>
+ <artifactId>spotbugs-maven-plugin</artifactId>
+ <configuration>
+ <omitVisitors>SharedVariableAtomicityDetector</omitVisitors>
Review Comment:
We have other spotbugs filters in src/main/spotbugs/exclude-filter.xml,
where we have fine-grained control over things to filter, and instead of
omitting entire detectors, we can filter based on the documented bug names. I'm
not sure which is better, but it would probably be nice to consolidate the
config, so we're doing it all in one place, instead of doing some here in the
pom, and others elsewhere in the exclude-filter.xml file.
##########
core/src/main/java/org/apache/accumulo/core/classloader/URLContextClassLoaderFactory.java:
##########
@@ -56,6 +56,12 @@ public URLContextClassLoaderFactory() {
}
}
+ @Override
+ @SuppressWarnings("deprecation")
+ public final void finalize() {
+ // Prevent finalizer attacks (SpotBugs CT_CONSTRUCTOR_THROW)
+ }
+
Review Comment:
Is this necessary if the class is final? It seems one OR the other is
necessary to fix this. We probably don't need to add the finalize method if it
is fixed another way.
##########
hadoop-mapreduce/pom.xml:
##########
@@ -84,6 +84,13 @@
</dependencies>
<build>
<plugins>
+ <plugin>
+ <groupId>com.github.spotbugs</groupId>
+ <artifactId>spotbugs-maven-plugin</artifactId>
+ <configuration>
+
<omitVisitors>SharedVariableAtomicityDetector,ConstructorThrow</omitVisitors>
Review Comment:
In addition to previous comments about consolidating spotbugs config rules,
this config here makes me wonder what rules are associated with
"SharedVariableAtomicityDetector" and "ConstructorThrow", and why we're
suppressing them here. It seems that suppression here is just because they are
noisy?
--
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]
