gianm commented on code in PR #16973:
URL: https://github.com/apache/druid/pull/16973#discussion_r1772671869
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -266,6 +339,41 @@ public boolean accept(File dir, String name)
}
}
+ private Optional<DruidExtensionDependencies>
getDruidExtensionDependencies(File extension)
+ {
+ final Collection<File> jars = FileUtils.listFiles(extension, new
String[]{"jar"}, false);
+ DruidExtensionDependencies druidExtensionDependencies = null;
+ for (File extensionFile : jars) {
+ try (JarFile jarFile = new JarFile(extensionFile.getPath())) {
+ Enumeration<JarEntry> entries = jarFile.entries();
+
+ while (entries.hasMoreElements()) {
+ JarEntry entry = entries.nextElement();
+ String entryName = entry.getName();
+ if (DRUID_EXTENSION_DEPENDENCIES_JSON.equals(entryName)) {
+ log.debug("Found extension dependency entry in druid jar %s",
extensionFile.getPath());
Review Comment:
Just "in jar" suffices, since we aren't filtering down to specifically Druid
jars anymore.
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -266,6 +339,41 @@ public boolean accept(File dir, String name)
}
}
+ private Optional<DruidExtensionDependencies>
getDruidExtensionDependencies(File extension)
+ {
+ final Collection<File> jars = FileUtils.listFiles(extension, new
String[]{"jar"}, false);
+ DruidExtensionDependencies druidExtensionDependencies = null;
+ for (File extensionFile : jars) {
+ try (JarFile jarFile = new JarFile(extensionFile.getPath())) {
+ Enumeration<JarEntry> entries = jarFile.entries();
+
+ while (entries.hasMoreElements()) {
+ JarEntry entry = entries.nextElement();
+ String entryName = entry.getName();
+ if (DRUID_EXTENSION_DEPENDENCIES_JSON.equals(entryName)) {
+ log.debug("Found extension dependency entry in druid jar %s",
extensionFile.getPath());
+ if (druidExtensionDependencies != null) {
+ throw new RE(
+ StringUtils.format(
+ "The extension [%s] has multiple druid jars with
dependencies in it. Each jar should be in a separate extension directory.",
+ extension.getName()
+ )
+ );
+ }
+ druidExtensionDependencies = objectMapper.readValue(
+ jarFile.getInputStream(entry),
+ DruidExtensionDependencies.class
+ );
+ }
+ }
+ }
+ catch (IOException e) {
+ throw new RuntimeException(e);
Review Comment:
Include the `extension` path in the error message, like `throw new RE(e,
"Failed to get dependencies for extension[%s]", extension);`
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -266,6 +339,41 @@ public boolean accept(File dir, String name)
}
}
+ private Optional<DruidExtensionDependencies>
getDruidExtensionDependencies(File extension)
+ {
+ final Collection<File> jars = FileUtils.listFiles(extension, new
String[]{"jar"}, false);
+ DruidExtensionDependencies druidExtensionDependencies = null;
+ for (File extensionFile : jars) {
+ try (JarFile jarFile = new JarFile(extensionFile.getPath())) {
+ Enumeration<JarEntry> entries = jarFile.entries();
+
+ while (entries.hasMoreElements()) {
+ JarEntry entry = entries.nextElement();
+ String entryName = entry.getName();
+ if (DRUID_EXTENSION_DEPENDENCIES_JSON.equals(entryName)) {
+ log.debug("Found extension dependency entry in druid jar %s",
extensionFile.getPath());
+ if (druidExtensionDependencies != null) {
+ throw new RE(
+ StringUtils.format(
+ "The extension [%s] has multiple druid jars with
dependencies in it. Each jar should be in a separate extension directory.",
Review Comment:
Same here, "multiple jars" is better than "multiple druid jars". Also, list
the jars? Otherwise the error message is going to be potentially confusing.
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -187,25 +201,84 @@ public File[] getExtensionFilesToLoad()
extensionsToLoad[i++] = extensionDir;
}
}
- return extensionsToLoad == null ? new File[]{} : extensionsToLoad;
+ extensionFilesToLoad = extensionsToLoad == null ? new File[]{} :
extensionsToLoad;
+ }
+
+ public File[] getExtensionFilesToLoad()
+ {
+ if (extensionFilesToLoad == null) {
+ initializeExtensionFilesToLoad();
+ }
+ return extensionFilesToLoad;
}
/**
* @param extension The File instance of the extension we want to load
*
* @return a URLClassLoader that loads all the jars on which the extension
is dependent
*/
- public URLClassLoader getClassLoaderForExtension(File extension, boolean
useExtensionClassloaderFirst)
+ public StandardURLClassLoader getClassLoaderForExtension(File extension,
boolean useExtensionClassloaderFirst)
{
- return loaders.computeIfAbsent(
- Pair.of(extension, useExtensionClassloaderFirst),
- k -> makeClassLoaderForExtension(k.lhs, k.rhs)
- );
+ return getClassLoaderForExtension(extension, useExtensionClassloaderFirst,
new ArrayList<>());
+ }
+
+ public StandardURLClassLoader getClassLoaderForExtension(File extension,
boolean useExtensionClassloaderFirst, List<String> extensionDependencyStack)
Review Comment:
Javadoc please, explaining what `extensionDependencyStack` is and how it's
used. It isn't obvious from the context.
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -187,25 +201,84 @@ public File[] getExtensionFilesToLoad()
extensionsToLoad[i++] = extensionDir;
}
}
- return extensionsToLoad == null ? new File[]{} : extensionsToLoad;
+ extensionFilesToLoad = extensionsToLoad == null ? new File[]{} :
extensionsToLoad;
+ }
+
+ public File[] getExtensionFilesToLoad()
+ {
+ if (extensionFilesToLoad == null) {
+ initializeExtensionFilesToLoad();
+ }
+ return extensionFilesToLoad;
}
/**
* @param extension The File instance of the extension we want to load
*
* @return a URLClassLoader that loads all the jars on which the extension
is dependent
*/
- public URLClassLoader getClassLoaderForExtension(File extension, boolean
useExtensionClassloaderFirst)
+ public StandardURLClassLoader getClassLoaderForExtension(File extension,
boolean useExtensionClassloaderFirst)
{
- return loaders.computeIfAbsent(
- Pair.of(extension, useExtensionClassloaderFirst),
- k -> makeClassLoaderForExtension(k.lhs, k.rhs)
- );
+ return getClassLoaderForExtension(extension, useExtensionClassloaderFirst,
new ArrayList<>());
+ }
+
+ public StandardURLClassLoader getClassLoaderForExtension(File extension,
boolean useExtensionClassloaderFirst, List<String> extensionDependencyStack)
+ {
+ Pair<File, Boolean> classLoaderKey = Pair.of(extension,
useExtensionClassloaderFirst);
+ StandardURLClassLoader classLoader = loaders.get(classLoaderKey);
+ if (classLoader == null) {
+ classLoader = makeClassLoaderWithDruidExtensionDependencies(extension,
useExtensionClassloaderFirst, extensionDependencyStack);
+ loaders.put(classLoaderKey, classLoader);
+ }
+
+ return classLoader;
+ }
+
+ private StandardURLClassLoader
makeClassLoaderWithDruidExtensionDependencies(File extension, boolean
useExtensionClassloaderFirst, List<String> extensionDependencyStack)
+ {
+ Optional<DruidExtensionDependencies> druidExtensionDependenciesOptional =
getDruidExtensionDependencies(extension);
+ List<String> druidExtensionDependenciesList =
druidExtensionDependenciesOptional.isPresent()
+ ?
druidExtensionDependenciesOptional.get().getDependsOnDruidExtensions()
+ : ImmutableList.of();
+
+ List<ClassLoader> extensionDependencyClassLoaders = new ArrayList<>();
+ for (String druidExtensionDependencyName : druidExtensionDependenciesList)
{
+ Optional<File> extensionDependencyFileOptional =
Arrays.stream(getExtensionFilesToLoad())
+ .filter(file -> file.getName().equals(druidExtensionDependencyName))
+ .findFirst();
+ if (!extensionDependencyFileOptional.isPresent()) {
+ throw new RE(
+ StringUtils.format(
+ "[%s] depends on [%s] which is not a valid extension or not
loaded.",
Review Comment:
`Extension[%s]` is better than just `[%s]`.
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -187,25 +201,84 @@ public File[] getExtensionFilesToLoad()
extensionsToLoad[i++] = extensionDir;
}
}
- return extensionsToLoad == null ? new File[]{} : extensionsToLoad;
+ extensionFilesToLoad = extensionsToLoad == null ? new File[]{} :
extensionsToLoad;
+ }
+
+ public File[] getExtensionFilesToLoad()
+ {
+ if (extensionFilesToLoad == null) {
+ initializeExtensionFilesToLoad();
+ }
+ return extensionFilesToLoad;
}
/**
* @param extension The File instance of the extension we want to load
*
* @return a URLClassLoader that loads all the jars on which the extension
is dependent
*/
- public URLClassLoader getClassLoaderForExtension(File extension, boolean
useExtensionClassloaderFirst)
+ public StandardURLClassLoader getClassLoaderForExtension(File extension,
boolean useExtensionClassloaderFirst)
{
- return loaders.computeIfAbsent(
- Pair.of(extension, useExtensionClassloaderFirst),
- k -> makeClassLoaderForExtension(k.lhs, k.rhs)
- );
+ return getClassLoaderForExtension(extension, useExtensionClassloaderFirst,
new ArrayList<>());
+ }
+
+ public StandardURLClassLoader getClassLoaderForExtension(File extension,
boolean useExtensionClassloaderFirst, List<String> extensionDependencyStack)
+ {
+ Pair<File, Boolean> classLoaderKey = Pair.of(extension,
useExtensionClassloaderFirst);
+ StandardURLClassLoader classLoader = loaders.get(classLoaderKey);
+ if (classLoader == null) {
+ classLoader = makeClassLoaderWithDruidExtensionDependencies(extension,
useExtensionClassloaderFirst, extensionDependencyStack);
+ loaders.put(classLoaderKey, classLoader);
+ }
+
+ return classLoader;
+ }
+
+ private StandardURLClassLoader
makeClassLoaderWithDruidExtensionDependencies(File extension, boolean
useExtensionClassloaderFirst, List<String> extensionDependencyStack)
+ {
+ Optional<DruidExtensionDependencies> druidExtensionDependenciesOptional =
getDruidExtensionDependencies(extension);
+ List<String> druidExtensionDependenciesList =
druidExtensionDependenciesOptional.isPresent()
+ ?
druidExtensionDependenciesOptional.get().getDependsOnDruidExtensions()
+ : ImmutableList.of();
+
+ List<ClassLoader> extensionDependencyClassLoaders = new ArrayList<>();
+ for (String druidExtensionDependencyName : druidExtensionDependenciesList)
{
+ Optional<File> extensionDependencyFileOptional =
Arrays.stream(getExtensionFilesToLoad())
+ .filter(file -> file.getName().equals(druidExtensionDependencyName))
+ .findFirst();
+ if (!extensionDependencyFileOptional.isPresent()) {
+ throw new RE(
+ StringUtils.format(
+ "[%s] depends on [%s] which is not a valid extension or not
loaded.",
+ extension.getName(),
+ druidExtensionDependencyName
+ )
+ );
+ }
+ File extensionDependencyFile = extensionDependencyFileOptional.get();
+ if
(extensionDependencyStack.contains(extensionDependencyFile.getName())) {
+ extensionDependencyStack.add(extensionDependencyFile.getName());
+ throw new RE(
+ StringUtils.format(
+ "[%s] has a circular druid extension dependency. Dependency
stack [%s].",
Review Comment:
`Extension[%s]` is better than just `[%s]`.
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -59,19 +67,25 @@
public class ExtensionsLoader
{
private static final Logger log = new Logger(ExtensionsLoader.class);
-
+ public static final String DRUID_EXTENSION_DEPENDENCIES_JSON =
"druid-extension-dependencies.json";
private final ExtensionsConfig extensionsConfig;
- private final ConcurrentHashMap<Pair<File, Boolean>, URLClassLoader> loaders
= new ConcurrentHashMap<>();
+ private final ObjectMapper objectMapper;
+
+ private final ConcurrentHashMap<Pair<File, Boolean>, StandardURLClassLoader>
loaders = new ConcurrentHashMap<>();
/**
* Map of loaded extensions, keyed by class (or interface).
*/
private final ConcurrentHashMap<Class<?>, Collection<?>> extensions = new
ConcurrentHashMap<>();
+ @MonotonicNonNull
+ private File[] extensionFilesToLoad;
Review Comment:
Thread-safety stance of this class is confusing: `loaders` and `extensions`
are `ConcurrentHashMap`, which suggests some thread-safety requirements,
whereas `extensionFilesToLoad` is not mutated in a thread-safe way. Also, this
patch changes mutation of `loaders` from `computeIfAbsent` (atomic) to `get` +
`put` (not atomic), which alters the thread-safety properties.
Anyway, I'm not sure if this class really needs to be thread-safe, but it
was in the past, so we might as well keep in that way. Looking at how it's
used, it isn't likely to be a point of thread contention, so it doesn't need
fancy thread-safety. A single lock we are `synchronized` around should be fine.
So I suggest making the `ConcurrentHashMap` into `HashMap`, marking all the
mutable fields as `@GuardedBy(this)`, and synchronizing around `this`.
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -266,6 +339,41 @@ public boolean accept(File dir, String name)
}
}
+ private Optional<DruidExtensionDependencies>
getDruidExtensionDependencies(File extension)
+ {
+ final Collection<File> jars = FileUtils.listFiles(extension, new
String[]{"jar"}, false);
+ DruidExtensionDependencies druidExtensionDependencies = null;
+ for (File extensionFile : jars) {
+ try (JarFile jarFile = new JarFile(extensionFile.getPath())) {
+ Enumeration<JarEntry> entries = jarFile.entries();
+
+ while (entries.hasMoreElements()) {
+ JarEntry entry = entries.nextElement();
+ String entryName = entry.getName();
+ if (DRUID_EXTENSION_DEPENDENCIES_JSON.equals(entryName)) {
+ log.debug("Found extension dependency entry in druid jar %s",
extensionFile.getPath());
Review Comment:
House style for error messages would include brackets around `%s`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
