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