[GitHub] [accumulo] keith-turner commented on a diff in pull request #3683: adds validation of classloader context

["keith-turner (via GitHub)" <gi...@apache.org>]

keith-turner commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1290659206


##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
 
   @SuppressWarnings("deprecation")
   public static ClassLoader getClassLoader(String context) {
-    try {
-      if (FACTORY.isValid(context)) {
-        return FACTORY.getClassLoader(context);
-      } else {
-        return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
-      }
-    } catch (RuntimeException e) {
+    if (context != null && !context.isEmpty()) {

Review Comment:
   > This check was removed in https://github.com/apache/accumulo/pull/3678 based on @ctubbsii comments https://github.com/apache/accumulo/pull/3678#discussion_r1285091831. We'll have to reconcile the difference of opinion I think.
   
   Thinking about that and this PR, there are two things I like about this PR.
   
    * It follows a principal of least surprise in a bug fix release.  It does not change behavior of existing code in subtle ways and add new SPIs.
    * There is no scope creep in the solution. Its a very targeted fix to the problem of the context not being verified when its set as property.



-- 
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