ctubbsii commented on code in PR #3136: URL: https://github.com/apache/accumulo/pull/3136#discussion_r1099852723
########## core/src/main/java/org/apache/accumulo/core/classloader/DefaultContextClassLoaderFactory.java: ########## @@ -48,55 +46,49 @@ public class DefaultContextClassLoaderFactory implements ContextClassLoaderFacto private static final Logger LOG = LoggerFactory.getLogger(DefaultContextClassLoaderFactory.class); private static final String className = DefaultContextClassLoaderFactory.class.getName(); - @SuppressWarnings("removal") - private static final Property VFS_CONTEXT_CLASSPATH_PROPERTY = - Property.VFS_CONTEXT_CLASSPATH_PROPERTY; + // Do we set a max size here and how long until we expire the classloader? + private final Cache<String,Context> contexts = + Caffeine.newBuilder().maximumSize(100).expireAfterAccess(1, TimeUnit.DAYS).build(); - public DefaultContextClassLoaderFactory(final AccumuloConfiguration accConf) { + public DefaultContextClassLoaderFactory() { if (!isInstantiated.compareAndSet(false, true)) { throw new IllegalStateException("Can only instantiate " + className + " once"); } - Supplier<Map<String,String>> contextConfigSupplier = - () -> accConf.getAllPropertiesWithPrefix(VFS_CONTEXT_CLASSPATH_PROPERTY); - setContextConfig(contextConfigSupplier); - LOG.debug("ContextManager configuration set"); - startCleanupThread(accConf, contextConfigSupplier); } - @SuppressWarnings("deprecation") - private static void setContextConfig(Supplier<Map<String,String>> contextConfigSupplier) { - org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader - .setContextConfig(contextConfigSupplier); - } + @Override + public ClassLoader getClassLoader(String contextName) { Review Comment: I was just calling it "context". The semantics of the context, whether it's a URL, a path, a name, etc., depends on the implementation. But generically, it's just a context, and the factory's job is to map a context to a ClassLoader, effectively equivalent to `Function<String,ClassLoader>`. You could call it `context` or `contextString` or similar. I agree it doesn't make sense to call it a `name`, though it could be a name for some implementations. Another thing about naming: this should not be called `DefaultContextClassLoaderFactory`. It should be named for how it behaves... not it's current status as the default. It will be very confusing (and I've seen this happen) if the default is changed, and it's no longer the default. So, for example, the default could change from `DefaultThing` to `MyThing`, and so then when people say `default thing`, you don't know if they mean the default `MyThing` or the non-default `DefaultThing`. In this case, it could just be `URLClassLoaderFactory` or `URLContextClassLoaderFactory`, and return instances of `URLClassLoader` objects, given a context that represents a list of URLs. -- 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-unsubscr...@accumulo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org