ctubbsii commented on code in PR #3678:
URL: https://github.com/apache/accumulo/pull/3678#discussion_r1285091993
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -82,6 +82,14 @@ public static ClassLoader getClassLoader(String context) {
}
}
+ public static boolean isValidContext(String context) {
+ if (context != null && !context.isEmpty()) {
+ return FACTORY.isValid(context);
Review Comment:
I think this is the only line needed here.
##########
server/base/src/main/java/org/apache/accumulo/server/util/PropUtil.java:
##########
@@ -65,6 +65,11 @@ protected static void validateProperties(final ServerContext
context,
}
throw new IllegalArgumentException(exceptionMessage + propStoreKey + "
name: "
+ prop.getKey() + ", value: " + prop.getValue());
+ } else if
(prop.getKey().equals(Property.TABLE_CLASSLOADER_CONTEXT.getKey())) {
+ if (!ClassLoaderUtil.isValidContext(prop.getValue())) {
Review Comment:
The else block can be removed and the indent promoted up one level to
simplify this. Also the two if conditions can be combined with `&&`
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -82,6 +82,14 @@ public static ClassLoader getClassLoader(String context) {
}
}
+ public static boolean isValidContext(String context) {
+ if (context != null && !context.isEmpty()) {
Review Comment:
This can be decided by the factory as well. There's no need to constrain
what the factory considers valid.
##########
core/src/main/java/org/apache/accumulo/core/spi/common/ContextClassLoaderFactory.java:
##########
@@ -65,4 +65,15 @@ default void init(ContextClassLoaderEnvironment env) {}
* @return the class loader for the given contextName
*/
ClassLoader getClassLoader(String contextName);
+
+ /**
+ * Validate that the contextName is supported by the
ContextClassLoaderFactory implementation
+ *
+ * @param contextName the name of the context that represents a class loader
that is managed by
+ * this factory (can be null)
+ * @return true if valid, false otherwise
+ */
+ default boolean isValid(String contextName) {
+ return false;
Review Comment:
Should the default be true to maintain existing behavior, or is existing
behavior so harmful as to warrant making all contexts invalid by default?
--
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]
