ctubbsii commented on code in PR #35:
URL:
https://github.com/apache/accumulo-classloaders/pull/35#discussion_r2636307720
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/LocalCachingContextClassLoaderFactory.java:
##########
@@ -266,10 +194,106 @@ public ClassLoader getClassLoader(final String
contextLocation)
} catch (RuntimeException e) {
throw new ContextClassLoaderException(e.getMessage(), e);
}
- if (newlyCreated.get()) {
- monitorContext(contextLocation, def.getMonitorIntervalSeconds());
+ return classloader.get();
+ }
+
+ private ContextDefinition computeDefinitionAndClassLoader(
+ AtomicReference<URLClassLoader> resultHolder, String contextLocation,
+ ContextDefinition previousDefinition) {
+ ContextDefinition computedDefinition;
+ if (previousDefinition == null) {
+ try {
+ computedDefinition = parseContextDefinition(contextLocation);
+ monitorContext(contextLocation,
computedDefinition.getMonitorIntervalSeconds());
+ } catch (ContextClassLoaderException e) {
+ throw new WrappedException(e);
+ }
+ } else {
+ computedDefinition = previousDefinition;
}
- return cl.get();
+ final URLClassLoader classloader = classloaders.computeIfAbsent(
+ computedDefinition.getContextName() + "-" +
computedDefinition.getChecksum(),
+ (Supplier<URLClassLoaderParams>) () -> {
+ try {
+ return localStore.get().storeContextResources(computedDefinition);
+ } catch (Exception e) {
+ throw new WrappedException(e);
+ }
+ });
+ resultHolder.set(classloader);
+ return computedDefinition;
+ }
+
+ private void checkMonitoredLocation(String contextLocation, long interval) {
+ ContextDefinition currentDef =
+ contextDefs.compute(contextLocation, (contextLocationKey,
previousDefinition) -> {
+ if (previousDefinition == null) {
+ return null;
+ }
+ if (!classloaders.anyMatch(k2 -> k2.substring(0, k2.lastIndexOf('-'))
+ .equals(previousDefinition.getContextName()))) {
+ // context has been removed from the map, no need to check for
update
+ LOG.debug("ClassLoader for context {} not present, no longer
monitoring for changes",
+ contextLocation);
+ return null;
+ }
+ return previousDefinition;
+ });
+ if (currentDef == null) {
+ // context has been removed from the map, no need to check for update
+ LOG.debug("ContextDefinition for context {} not present, no longer
monitoring for changes",
+ contextLocation);
+ return;
+ }
+ long nextInterval = interval;
+ try {
+ final ContextDefinition update = parseContextDefinition(contextLocation);
+ if (!currentDef.getChecksum().equals(update.getChecksum())) {
+ LOG.debug("Context definition for {} has changed", contextLocation);
+ if (!currentDef.getContextName().equals(update.getContextName())) {
+ LOG.warn(
+ "Context name changed for context {}, but context cache
directory will remain {} (old={}, new={})",
+ contextLocation, currentDef.getContextName(),
currentDef.getContextName(),
Review Comment:
Yeah, this will be deleted before I'm done. But, I want to be careful to
make sure I handle the contextName contained in the definition file correctly.
I'm also not sure we need this context name in the file at all.
Currently, we keep the contextdefinition, and its locally cached resources,
monitored and updated based on the source URL, not the context name in the
file. However, I do use the context name for two purposes, still:
1. it's a portion of the key, along with the hash of the defintion contents,
for the deduplication cache that stores the URLClassLoader instances, and
2. it's used for the local copy of the context defintion downloaded to the
contexts/ directory
I'm thinking that it may also be useful for a 3rd thing: to show which
contexts are currently in use.
However, all of this could be replaced with the name of the file itself,
from the remote source URL. We don't actually need a separate name serialized
in the file.
--
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]
