dlmarion commented on code in PR #3136:
URL: https://github.com/apache/accumulo/pull/3136#discussion_r1091857742
##########
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) {
+ if (contextName == null) {
+ throw new IllegalArgumentException("Unknown context");
+ }
- private static void startCleanupThread(final AccumuloConfiguration conf,
- final Supplier<Map<String,String>> contextConfigSupplier) {
- ScheduledFuture<?> future = ThreadPools.getClientThreadPools((t, e) -> {
- LOG.error("context classloader cleanup thread has failed.", e);
- }).createGeneralScheduledExecutorService(conf)
- .scheduleWithFixedDelay(Threads.createNamedRunnable(className +
"-cleanup", () -> {
- LOG.trace("{}-cleanup thread, properties: {}", className, conf);
- Set<String> contextsInUse =
contextConfigSupplier.get().keySet().stream()
- .map(p ->
p.substring(VFS_CONTEXT_CLASSPATH_PROPERTY.getKey().length()))
- .collect(Collectors.toSet());
- LOG.trace("{}-cleanup thread, contexts in use: {}", className,
contextsInUse);
- removeUnusedContexts(contextsInUse);
- }), 1, 1, MINUTES);
- ThreadPools.watchNonCriticalScheduledTask(future);
- LOG.debug("Context cleanup timer started at 60s intervals");
- }
+ final ClassLoader loader = contexts.get(contextName,
Context::new).getClassLoader();
- @SuppressWarnings("deprecation")
- private static void removeUnusedContexts(Set<String> contextsInUse) {
- org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader
- .removeUnusedContexts(contextsInUse);
+ LOG.debug("Returning classloader {} for context {}",
loader.getClass().getName(), contextName);
+ return loader;
}
- @SuppressWarnings("deprecation")
- @Override
- public ClassLoader getClassLoader(String contextName) {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader
- .getContextClassLoader(contextName);
+ private static class Context {
Review Comment:
I understand the need for Caffeine, I'm just not sure we need this class. We
could change the definition of the Caffeine map from <String,Context> to
<String,Classloader>.
--
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]
