dlmarion commented on code in PR #35:
URL:
https://github.com/apache/accumulo-classloaders/pull/35#discussion_r2636348890
##########
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(),
+ update.getContextName());
+ }
+ localStore.get().storeContextResources(update);
+ contextDefs.put(contextLocation, update);
+ nextInterval = update.getMonitorIntervalSeconds();
+ classloaderFailures.remove(contextLocation);
+ } else {
+ LOG.trace("Context definition for {} has not changed",
contextLocation);
+ }
+ } catch (ContextClassLoaderException | InterruptedException | IOException
| URISyntaxException
+ | RuntimeException e) {
+ LOG.error("Error parsing updated context definition at {}. Classloader
NOT updated!",
+ contextLocation, e);
+ final Timer failureTimer = classloaderFailures.get(contextLocation);
+ if (updateFailureGracePeriodMins.isZero()) {
+ // failure monitoring is disabled
+ LOG.debug("Property {} not set, not tracking classloader failures for
context {}",
Review Comment:
I think it was `debug` before your changes, looking at the code again I
think we should make it `warn`.
--
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]
