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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
