dlmarion commented on code in PR #35:
URL:
https://github.com/apache/accumulo-classloaders/pull/35#discussion_r2626921141
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/cache/CacheUtils.java:
##########
@@ -126,4 +146,101 @@ public static LockInfo lockContextCacheDir(final Path
contextCacheDir)
}
}
+ public static URLClassLoaderHelper stageContext(final Path baseCacheDir,
+ final ContextDefinition contextDefinition)
+ throws IOException, ContextClassLoaderException, InterruptedException,
URISyntaxException {
+ final RetryFactory retryFactory =
+ Retry.builder().infiniteRetries().retryAfter(1, TimeUnit.SECONDS)
+ .incrementBy(1, TimeUnit.SECONDS).maxWait(5,
TimeUnit.MINUTES).backOffFactor(2)
+ .logInterval(1, TimeUnit.SECONDS).createFactory();
+ requireNonNull(contextDefinition, "definition must be supplied");
+ CacheUtils.createBaseDirs(baseCacheDir);
+ Path contextsDir = CacheUtils.contextsDir(baseCacheDir);
+ final Set<File> localFiles = new LinkedHashSet<>();
+ try {
+ LockInfo lockInfo = CacheUtils.lockContextCacheDir(contextsDir);
+ Files.write(
+ contextsDir
+ .resolve(contextDefinition.getContextName() + "_" +
contextDefinition.getChecksum()),
+ contextDefinition.toJson().getBytes(UTF_8));
+ while (lockInfo == null) {
+ // something else is updating this directory
+ LOG.info("Directory {} locked, another process must be updating the
class loader contents. "
+ + "Retrying in 1 second", contextsDir);
+ Thread.sleep(1000);
+ lockInfo = CacheUtils.lockContextCacheDir(contextsDir);
+ }
+ Path resourcesDir = CacheUtils.resourcesDir(baseCacheDir);
+ try {
+ for (Resource updatedResource : contextDefinition.getResources()) {
+ File file = cacheResource(retryFactory, resourcesDir,
updatedResource);
+ localFiles.add(file.getAbsoluteFile());
+ LOG.trace("Added element {} to classpath", file);
+ }
+ } finally {
+ lockInfo.unlock();
+ }
+ } catch (Exception e) {
+ LOG.error("Error initializing context: " +
contextDefinition.getContextName(), e);
+ throw e;
+ }
+ return new URLClassLoaderHelper(
+ contextDefinition.getContextName() + "_" +
contextDefinition.getChecksum(),
+ localFiles.stream().map(File::toURI).map(t -> {
+ try {
+ return t.toURL();
+ } catch (MalformedURLException e) {
+ // this shouldn't happen since these are local files
+ throw new UncheckedIOException(e);
+ }
+ }).toArray(URL[]::new));
+ }
+
+ private static File cacheResource(final RetryFactory retryFactory, final
Path contextCacheDir,
Review Comment:
```suggestion
private static File cacheResource(final RetryFactory retryFactory, final
Path resourceCacheDir,
```
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/cache/CacheUtils.java:
##########
@@ -73,26 +92,27 @@ public void unlock() throws IOException {
}
- private static Path mkdir(final Path p) throws IOException {
- try {
- return Files.createDirectory(p, PERMISSIONS);
- } catch (FileAlreadyExistsException e) {
- return p;
+ public static void createBaseDirs(final Path baseDir)
+ throws IOException, ContextClassLoaderException {
+ if (baseDir == null) {
+ throw new ContextClassLoaderException("received null for cache
directory");
}
+ var contextDir = baseDir.resolve("contexts");
+ Files.createDirectories(contextDir, PERMISSIONS);
Review Comment:
Is the intent here that the base cache dir will get created if it doesn't
exist as a side-effect of creating the context directory? The old code created
the base cache dir explicitly. I can't determine if the new code is doing the
same (implicility) or if the intent is to fail if the base cache dir is not
created first by the user.
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/cache/CacheUtils.java:
##########
@@ -126,4 +146,101 @@ public static LockInfo lockContextCacheDir(final Path
contextCacheDir)
}
}
+ public static URLClassLoaderHelper stageContext(final Path baseCacheDir,
Review Comment:
Suggest renaming this method to something like `cacheContextResources`. It
will make it more clear what is happening when it's called in other parts of
the codebase. When I first saw `stage` I thought something temporary was
happening, like a `staging area`.
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/definition/Resource.java:
##########
@@ -18,37 +18,32 @@
*/
package org.apache.accumulo.classloader.lcc.definition;
-import java.net.MalformedURLException;
import java.net.URL;
import java.util.Objects;
-public class Resource implements Comparable<Resource> {
+public class Resource {
- private String location;
+ private URL location;
Review Comment:
This is good, I wasn't sure if gson supported serializing/deserializing URLs
without an adapter.
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/cache/CacheUtils.java:
##########
@@ -126,4 +146,101 @@ public static LockInfo lockContextCacheDir(final Path
contextCacheDir)
}
}
+ public static URLClassLoaderHelper stageContext(final Path baseCacheDir,
+ final ContextDefinition contextDefinition)
+ throws IOException, ContextClassLoaderException, InterruptedException,
URISyntaxException {
+ final RetryFactory retryFactory =
+ Retry.builder().infiniteRetries().retryAfter(1, TimeUnit.SECONDS)
+ .incrementBy(1, TimeUnit.SECONDS).maxWait(5,
TimeUnit.MINUTES).backOffFactor(2)
+ .logInterval(1, TimeUnit.SECONDS).createFactory();
+ requireNonNull(contextDefinition, "definition must be supplied");
+ CacheUtils.createBaseDirs(baseCacheDir);
+ Path contextsDir = CacheUtils.contextsDir(baseCacheDir);
+ final Set<File> localFiles = new LinkedHashSet<>();
+ try {
+ LockInfo lockInfo = CacheUtils.lockContextCacheDir(contextsDir);
+ Files.write(
+ contextsDir
+ .resolve(contextDefinition.getContextName() + "_" +
contextDefinition.getChecksum()),
+ contextDefinition.toJson().getBytes(UTF_8));
Review Comment:
I think this needs to move down to after the while loop. If lockInfo is null
here, then this process does not have the lock.
##########
modules/local-caching-classloader/src/test/java/org/apache/accumulo/classloader/lcc/cache/CacheUtilsTest.java:
##########
@@ -137,11 +195,78 @@ public void testLock() throws Exception {
lockInfo.unlock();
}
} finally {
- Files.delete(cx1.resolve("lock_file"));
- Files.delete(cx1);
- Files.delete(base);
+ Files.delete(contextsDir.resolve("lock_file"));
+ }
+
+ }
+
+ @Test
Review Comment:
I wonder if the tests below make more sense in the Factory test file.
##########
modules/local-caching-classloader/src/main/java/org/apache/accumulo/classloader/lcc/util/URLClassLoaderHelper.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.classloader.lcc.util;
+
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.util.Arrays;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+/**
+ * A simple utility class to hold the parameters necessary for constructing a
URLClassLoader and a
+ * convenience method to do the construction. This exists to hold the
URLClassLoader name and URLs
+ * in a single object, so it can be provided by a single Supplier.
+ */
+public class URLClassLoaderHelper {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(URLClassLoaderHelper.class);
+
+ private final String name;
+ private final URL[] urls;
+
+ public URLClassLoaderHelper(final String name, final URL[] urls) {
+ this.name = name;
+ this.urls = Arrays.copyOf(urls, urls.length);
+ }
+
+ @SuppressFBWarnings(value = "DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED",
+ justification = "doPrivileged is deprecated without replacement and
removed in newer Java")
+ public URLClassLoader createClassLoader() {
+ final var cl = new URLClassLoader(name, urls, getClass().getClassLoader());
+ if (LOG.isTraceEnabled()) {
+ LOG.trace("New classloader created from URLs: {}", Arrays.asList(urls));
Review Comment:
```suggestion
LOG.trace("New classloader created for {} from URLs: {}", name,
Arrays.asList(urls));
```
##########
modules/local-caching-classloader/src/test/java/org/apache/accumulo/classloader/lcc/LocalCachingContextClassLoaderFactoryTest.java:
##########
@@ -269,36 +271,39 @@ public void testInitialNonExistentResource() throws
Exception {
"InitialContextDefinitionFileMissingResource.json", def.toJson());
final URL initialDefUrl = new URL(fs.getUri().toString() +
initial.toUri().toString());
- ContextClassLoaderException ex =
assertThrows(ContextClassLoaderException.class,
+ var ex = assertThrows(ContextClassLoaderException.class,
() -> FACTORY.getClassLoader(initialDefUrl.toString()));
assertTrue(ex.getMessage().endsWith("jarACopy.jar does not exist."));
}
@Test
public void testInitialBadResourceURL() throws Exception {
- // remove the file:// prefix from the URL
- Resource r = new Resource(jarAOrigLocation.toString().substring(6),
"1234");
LinkedHashSet<Resource> resources = new LinkedHashSet<>();
- resources.add(r);
+ resources.add(new Resource(jarAOrigLocation, "1234"));
- ContextDefinition def = new ContextDefinition("initial",
MONITOR_INTERVAL_SECS, resources);
+ // remove the file:// prefix from the URL
+ String goodJson = new ContextDefinition("initial", MONITOR_INTERVAL_SECS,
resources).toJson();
+ String badJson =
+ goodJson.replace(jarAOrigLocation.toString(),
jarAOrigLocation.toString().substring(6));
+ assertNotEquals(goodJson, badJson);
Review Comment:
Are we doing anything else with `goodJson`? Is this being done this way
because you can't create an invalid URL when constructing the Resource?
--
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]
