cshannon commented on code in PR #3136:
URL: https://github.com/apache/accumulo/pull/3136#discussion_r1095773211
##########
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:
Ah yeah sorry, I misread what you were saying. The Context class could be
removed and I'll do that, it was a hold over from before. I had kept it in case
for some reason we wanted to add more metadata to what is cached but storing
the ClassLoader as the value should be fine as I can't really think of a good
reason we need to have anything else cached and it could always be changed
later.
--
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]
