dlmarion commented on code in PR #3136:
URL: https://github.com/apache/accumulo/pull/3136#discussion_r1086742635
##########
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:
Given that we are changing the semantics of this from a name to a comma
separated list of URIs, maybe we should change the name of the input parameter
from `contextName` to `contextValue` or something. Both here and in the
interface. Thoughts?
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ClasspathCommand.java:
##########
@@ -30,9 +30,9 @@ public class ClasspathCommand extends Command {
public int execute(final String fullCommand, final CommandLine cl, final
Shell shellState) {
final PrintWriter writer = shellState.getWriter();
-
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.printClassPath(s
-> {
- writer.print(s);
- }, true);
+ //
org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.printClassPath(s
-> {
+ // writer.print(s);
+ // }, true);
Review Comment:
So, I think that it could print `java.class.path` (of the shell, not the
server, unless they are the same) and then print out all of the values of the
`table.class.loader.context` properties defined on the tables.
##########
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:
Do we need this class? Could we just make the Caffeine map
<String,ClassLoader> ?
##########
start/src/main/java/org/apache/accumulo/start/Main.java:
##########
@@ -45,13 +44,11 @@ public static void main(final String[] args) throws
Exception {
// Preload classes that cause a deadlock between the ServiceLoader and the
DFSClient when
// using the VFSClassLoader with jars in HDFS.
ClassLoader loader = getClassLoader();
Review Comment:
Do we need the `getClassLoader()` method anymore? Can we just call
`ClassLoaderUtil.getClassLoader(null);` ?
--
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]
