You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "DomGarguilo (via GitHub)" <gi...@apache.org> on 2025/08/07 18:41:31 UTC
[PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
DomGarguilo opened a new pull request, #5785:
URL: https://github.com/apache/accumulo/pull/5785
This PR fixes the warnings from the core module. Here is a description from the ticket:
> This warning gets emitted when an exception is thrown within an constructor.
>
> There are several ways to avoid this warning:
>
> 1. make the class final
> 2. remove the thrown exception from the constructor
> 3. add a `finalize()` method so it cant be overridden
> 4. make the constructor private
>
> There are pros and cons to each fix.
>
> * making a class final prohibits inheriting it
> * removing the thrown exception usually means forgoing validation
> * adding a finalize() method means we also need to ignore the deprecation warning since finalizer() is deprecated
> * making the constructor private usually takes a lot of refactoring to move to a different pattern to create the object
>
> I think the best approach here is to work through a couple modules at a time and fix these.
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#issuecomment-3168584821
I think feedback regarding the method of fixing this warning would be helpful (making the class final vs. adding the `finalize()` method). I tried to only make things final if it was an inner class and/or something that seemed very unlikely to be subclassed, however I just used my best judgement so would like others input on this part.
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#issuecomment-3189029454
This PR is no longer a WIP.
To fix these spotbugs warnings I either made the class final (best solution, not possible everywhere) or I added a `@SuppressFBWarnings` tag to the class.
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#discussion_r2353225677
##########
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:
> Do you think that sounds good or would do you think its best to suppress this at module/project level?
I think trying those 3 fixes, in that order, are probably best. There may be good reasons to suppress at the module/project level, but it should be a very good reason. We have relatively few module/project-wide exceptions, and they are easily articulable, like the comments in the existing `spotbugs/exclude-filter.xml` files.
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#discussion_r2311716873
##########
iterator-test-harness/pom.xml:
##########
@@ -65,4 +65,15 @@
<scope>test</scope>
</dependency>
</dependencies>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>com.github.spotbugs</groupId>
+ <artifactId>spotbugs-maven-plugin</artifactId>
+ <configuration>
+ <omitVisitors>SharedVariableAtomicityDetector,ConstructorThrow</omitVisitors>
+ </configuration>
+ </plugin>
Review Comment:
It would be much simpler to just set `<spotbugs.omitVisitors />` in the property section of the various POMs rather than add the whole plugin config to the pom.
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#issuecomment-3168562346
> Do we need to make release notes changes for finalizing any of the classes that are defined in our public API?
That is a good question. I am not sure.
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#issuecomment-3168605446
> > Do we need to make release notes changes for finalizing any of the classes that are defined in our public API?
>
> That is a good question. I am not sure.
> I think feedback regarding the method of fixing this warning would be helpful (making the class final vs. adding the `finalize()` method). I tried to only make things final if it was an inner class and/or something that seemed very unlikely to be subclassed, however I just used my best judgement so would like others input on this part.
I don't see an issue with finalizing the default implementation classes since users should be implementing new classes from the interface rather than extending the clientImpl classes.
Since finalize() is deprecated, does this SpotBugs warning go away if we were to switch to a newer java version?
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#discussion_r2349844006
##########
iterator-test-harness/pom.xml:
##########
@@ -65,4 +65,15 @@
<scope>test</scope>
</dependency>
</dependencies>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>com.github.spotbugs</groupId>
+ <artifactId>spotbugs-maven-plugin</artifactId>
+ <configuration>
+ <omitVisitors>SharedVariableAtomicityDetector,ConstructorThrow</omitVisitors>
+ </configuration>
+ </plugin>
Review Comment:
Made this change in 941c2a3 @ctubbsii. Definitely a better approach than what I had before.
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#issuecomment-3169079777
> I think feedback regarding the method of fixing this warning would be helpful (making the class final vs. adding the finalize() method).
I have no issue with making classes final where we can and where it makes sense. I'm not sure adding a finalizer to fix the others is a good long-term approach though it's likely the easiest fix. Finalizers themselves are [deprecated](https://openjdk.org/jeps/421) and will be removed in a future Java release. I'm wondering if fixing the objects so that they don't throw exceptions from the constructor is the better choice, for example, using an `init` method on the objects that needs to be called post-construction but before use. There is now a jvm option, `--finalization=disabled`, in JDK 18 that disables all Finalizers as a step toward removal.
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#discussion_r2299015550
##########
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:
Avoding finalize here will be helpful in the future https://openjdk.org/jeps/421
##########
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:
Avoiding finalize here will be helpful in the future https://openjdk.org/jeps/421
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#discussion_r2547132907
##########
core/src/main/java/org/apache/accumulo/core/iteratorsImpl/conf/ColumnSet.java:
##########
@@ -30,6 +30,10 @@
import org.apache.accumulo.core.util.Pair;
import org.apache.hadoop.io.Text;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+@SuppressFBWarnings(value = "CT_CONSTRUCTOR_THROW",
+ justification = "Constructor validation is required for proper initialization")
Review Comment:
I went ahead and made `iteratorsImpl/conf/ColumnSet.java` final.
Looked through for more classes that are in this bucket. Found and made final:
clientImpl/OfflineIterator.java
clientImpl/bulk/LoadMappingIterator.java
dataImpl/KeyExtent.java
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#issuecomment-3700168398
I am going to go ahead and merge this since its been approved and all feedback has been addressed
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#discussion_r2310567572
##########
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:
I'm not sure i understand your question about why we're suppressing them here. Do you mean within that module specifically or within the poms in general?
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#discussion_r2308576714
##########
iterator-test-harness/pom.xml:
##########
@@ -65,4 +65,15 @@
<scope>test</scope>
</dependency>
</dependencies>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>com.github.spotbugs</groupId>
+ <artifactId>spotbugs-maven-plugin</artifactId>
+ <configuration>
+ <omitVisitors>SharedVariableAtomicityDetector,ConstructorThrow</omitVisitors>
+ </configuration>
+ </plugin>
Review Comment:
Could these changes be made in the accumulo parent pom instead of in each pom?
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#discussion_r2353237776
##########
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:
It's only temporary while it's in the PR. Once it gets merged, it's more or less permanent, or at least as permanent as the exclude-filter.xml files. My review comments are based on the desired end result after this is merged. That is to say, doing it in a different way while you're working on it is fine... but unless there's a good reason to have these configured separately from what's in the exclude-filter.xml files, then it's better that the end result by the time this is merged, has removed these extra configs, or consolidated them with the exclude-filter.xml files, so we don't have config scattered everywhere that basically does similar things.
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#discussion_r2310557934
##########
iterator-test-harness/pom.xml:
##########
@@ -65,4 +65,15 @@
<scope>test</scope>
</dependency>
</dependencies>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>com.github.spotbugs</groupId>
+ <artifactId>spotbugs-maven-plugin</artifactId>
+ <configuration>
+ <omitVisitors>SharedVariableAtomicityDetector,ConstructorThrow</omitVisitors>
+ </configuration>
+ </plugin>
Review Comment:
They could but the point of doing it in each pom is to allow us to make these changes incrementally so we don't have to fix the whole codebase at once and instead can go module by module.
--
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
Re: [PR] WIP - Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#discussion_r2274400160
##########
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:
Yea I agree. Adding the finalize method definitely seems like the worst fix.
I think the best way to handle things is
1. if its easy and safe to remove the exception thrown from the constructor, do that (usually not possible)
2. try to make the class final if its obvious that it can be made final
3. add a `@SuppressFBWarnings` tag to the place in the code that the warning is coming from
Do you think that sounds good or would do you think its best to suppress this at module/project level?
--
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
Re: [PR] WIP - Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#discussion_r2274381697
##########
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:
I would argue that each serves a different purpose. To me it seems the filters in exclude-filter.xml are more or less permanent for code that we don't expect to change where as this new `<omitVisitors>` approach is a nice way to temporarily omit these detectors while we are fixing them. If we find that we want to permanently omit these detectors than I think it would be a good idea to move things into the xml file but for now this seems to work well.
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#discussion_r2353273272
##########
core/src/main/java/org/apache/accumulo/core/iteratorsImpl/conf/ColumnSet.java:
##########
@@ -30,6 +30,10 @@
import org.apache.accumulo.core.util.Pair;
import org.apache.hadoop.io.Text;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+@SuppressFBWarnings(value = "CT_CONSTRUCTOR_THROW",
+ justification = "Constructor validation is required for proper initialization")
Review Comment:
While we may be reluctant to make final public API classes that the user might have extended, stuff in `*Impl` are fair game. I'm not sure if it's possible to make them all `final` (we might extend them ourselves, elsewhere internally), but if it is possible, there shouldn't be any harm doing so.
##########
server/base/pom.xml:
##########
@@ -144,6 +147,7 @@
<directory>src/test/resources</directory>
</testResource>
</testResources>
+ <plugins />
Review Comment:
```suggestion
```
##########
server/compactor/pom.xml:
##########
@@ -88,5 +91,6 @@
</plugin>
</plugins>
</pluginManagement>
+ <plugins />
Review Comment:
```suggestion
```
##########
iterator-test-harness/pom.xml:
##########
@@ -65,4 +68,7 @@
<scope>test</scope>
</dependency>
</dependencies>
+ <build>
+ <plugins />
+ </build>
Review Comment:
```suggestion
```
##########
server/manager/pom.xml:
##########
@@ -126,4 +129,7 @@
<scope>test</scope>
</dependency>
</dependencies>
+ <build>
+ <plugins />
+ </build>
Review Comment:
```suggestion
```
##########
shell/pom.xml:
##########
@@ -112,4 +115,7 @@
<scope>test</scope>
</dependency>
</dependencies>
+ <build>
+ <plugins />
+ </build>
Review Comment:
```suggestion
```
##########
server/gc/pom.xml:
##########
@@ -101,4 +104,7 @@
<scope>test</scope>
</dependency>
</dependencies>
+ <build>
+ <plugins />
+ </build>
Review Comment:
```suggestion
```
##########
core/src/main/java/org/apache/accumulo/core/classloader/URLContextClassLoaderFactory.java:
##########
@@ -37,7 +37,7 @@
* URLClassLoader based on the given context value which is a CSV list of URLs. For example,
* file://path/one/jar1.jar,file://path/two/jar2.jar
*/
-public class URLContextClassLoaderFactory implements ContextClassLoaderFactory {
+public final class URLContextClassLoaderFactory implements ContextClassLoaderFactory {
Review Comment:
This class actually seems like it might be useful to extend. The check in the constructor probably doesn't need to be there, and is overly restrictive. It's only there as a sanity check to make sure we didn't screw up elsewhere in Accumulo... but having it here isn't exactly the right place for that check, and is overly restrictive.
##########
server/tserver/pom.xml:
##########
@@ -167,6 +170,7 @@
</plugin>
</plugins>
</pluginManagement>
+ <plugins />
Review Comment:
```suggestion
```
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#discussion_r2353207369
##########
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:
I mean, having the config here raises the question "why are we skipping these checks?" without bothering to provide an answer. In the other places, such as the annotation, or the XML file, we generally offer an explanation, but there isn't one here. It's not clear what checks are being skipped, and why, when these visitors are being omitted.
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo merged PR #5785:
URL: https://github.com/apache/accumulo/pull/5785
--
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
Re: [PR] Fix ConstructorThrow SpotBugs warnings in core module [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #5785:
URL: https://github.com/apache/accumulo/pull/5785#issuecomment-3702950777
Ran into issues merging #5998 after this PR was merged into main.
I added the error suppression to `MultiIterator` to allow spotbugs to work and removed the `final` declaration as `MultiShuffledIterator` extends `MultiIterator`.
--
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