gianm commented on code in PR #16973:
URL: https://github.com/apache/druid/pull/16973#discussion_r1755016909
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -290,13 +302,44 @@ private void addAllFromCurrentClassLoader()
private void addAllFromFileSystem()
{
+ Map<String, URLClassLoader> extensionClassLoaderMap = new HashMap<>();
+
+ for (File extension : getExtensionFilesToLoad()) {
+ extensionClassLoaderMap.put(
+ extension.getName(),
+ getClassLoaderForExtension(
+ extension,
+ extensionsConfig.isUseExtensionClassloaderFirst()
+ )
+ );
+ }
+
for (File extension : getExtensionFilesToLoad()) {
Review Comment:
It would be cleaner to iterate `extensionClassLoaderMap.entrySet()` here so
we don't need to re-read the extensions directory and re-call
`getClassLoaderForExtension`.
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -59,8 +68,10 @@
public class ExtensionsLoader
{
private static final Logger log = new Logger(ExtensionsLoader.class);
-
+ public static final String EXTENSION_DEPENDENCIES_JSON =
"extension-dependencies.json";
Review Comment:
Should have `druid` somewhere in the name, like
`druid-extension-dependencies.json`. It should also be in `resources/` inside
the jar.
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -290,13 +302,44 @@ private void addAllFromCurrentClassLoader()
private void addAllFromFileSystem()
{
+ Map<String, URLClassLoader> extensionClassLoaderMap = new HashMap<>();
+
+ for (File extension : getExtensionFilesToLoad()) {
+ extensionClassLoaderMap.put(
+ extension.getName(),
+ getClassLoaderForExtension(
+ extension,
+ extensionsConfig.isUseExtensionClassloaderFirst()
+ )
+ );
+ }
+
for (File extension : getExtensionFilesToLoad()) {
log.debug("Loading extension [%s] for class [%s]",
extension.getName(), serviceClass);
try {
final URLClassLoader loader = getClassLoaderForExtension(
extension,
extensionsConfig.isUseExtensionClassloaderFirst()
);
+ Optional<ExtensionDependencies> extensionDependencies =
getExtensionDependencies(loader);
+ List<String> extensionDruidExtensionDependencies =
extensionDependencies.isPresent()
+ ? extensionDependencies.get().getDependsOnDruidExtensions()
+ : ImmutableList.of();
+
+ List<ClassLoader> chainedClassLoadersForExtension = new
ArrayList<>();
+ for (String druidExtensionDependency :
extensionDruidExtensionDependencies) {
+ if
(!extensionClassLoaderMap.containsKey(druidExtensionDependency)) {
+ throw new RE(
+ StringUtils.format(
+ "%s depends on %s which is not a valid extension or not
loaded.",
+ extension.getName(),
+ druidExtensionDependency
+ )
+ );
+ }
+
chainedClassLoadersForExtension.add(extensionClassLoaderMap.get(druidExtensionDependency));
+ }
+ ((StandardClassLoader)
loader).setExtensionDependencyClassLoaders(chainedClassLoadersForExtension);
Review Comment:
This is a little confusing, so it should be explained with a comment
somewhere. Perhaps as javadoc on `addAllFromFileSystem()` itself. The comment
should call out that what happens is first we create classloaders for each
extension directory, then we loop through and modify them to link in the
dependency classloaders.
This should also be called out on the javadoc for
`getClassLoaderForExtension`, since callers of that would need to know that
dependency classloaders aren't necessarily linked in. (It depends on whether
`getFromExtensions()` was called too.)
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -290,13 +302,44 @@ private void addAllFromCurrentClassLoader()
private void addAllFromFileSystem()
{
+ Map<String, URLClassLoader> extensionClassLoaderMap = new HashMap<>();
+
+ for (File extension : getExtensionFilesToLoad()) {
+ extensionClassLoaderMap.put(
+ extension.getName(),
+ getClassLoaderForExtension(
+ extension,
+ extensionsConfig.isUseExtensionClassloaderFirst()
+ )
+ );
+ }
+
for (File extension : getExtensionFilesToLoad()) {
log.debug("Loading extension [%s] for class [%s]",
extension.getName(), serviceClass);
try {
final URLClassLoader loader = getClassLoaderForExtension(
extension,
extensionsConfig.isUseExtensionClassloaderFirst()
);
+ Optional<ExtensionDependencies> extensionDependencies =
getExtensionDependencies(loader);
+ List<String> extensionDruidExtensionDependencies =
extensionDependencies.isPresent()
+ ? extensionDependencies.get().getDependsOnDruidExtensions()
+ : ImmutableList.of();
+
+ List<ClassLoader> chainedClassLoadersForExtension = new
ArrayList<>();
+ for (String druidExtensionDependency :
extensionDruidExtensionDependencies) {
+ if
(!extensionClassLoaderMap.containsKey(druidExtensionDependency)) {
+ throw new RE(
+ StringUtils.format(
+ "%s depends on %s which is not a valid extension or not
loaded.",
+ extension.getName(),
+ druidExtensionDependency
+ )
+ );
+ }
+
chainedClassLoadersForExtension.add(extensionClassLoaderMap.get(druidExtensionDependency));
+ }
+ ((StandardClassLoader)
loader).setExtensionDependencyClassLoaders(chainedClassLoadersForExtension);
Review Comment:
Actually this is so weird that we should try to change it. It creates a
situation where the classloader you get may not be usable unless you also call
`getFromExtensions()`, which is weird. Perhaps we can deal with this by making
`getClassLoaderForExtension` private, and then adding a new method that gets
the jar paths (not a classloader), which is what the Hadoop task wants anyway.
And which doesn't have these problems.
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -334,5 +377,34 @@ private void tryAdd(T serviceImpl, String extensionType)
implsToLoad.add(serviceImpl);
}
}
+
+ private Optional<ExtensionDependencies>
getExtensionDependencies(URLClassLoader loader)
+ {
+ for (URL url : loader.getURLs()) {
+ File jarFileLocation = new File(url.getPath());
+ if (jarFileLocation.getName().startsWith("druid")) {
Review Comment:
This is IMO too magical; better to look at all jars. If there is more than
one jar with the file then throw an error. Third party extensions especially
may not start with `druid`.
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -334,5 +377,34 @@ private void tryAdd(T serviceImpl, String extensionType)
implsToLoad.add(serviceImpl);
}
}
+
+ private Optional<ExtensionDependencies>
getExtensionDependencies(URLClassLoader loader)
+ {
+ for (URL url : loader.getURLs()) {
+ File jarFileLocation = new File(url.getPath());
+ if (jarFileLocation.getName().startsWith("druid")) {
+ try (JarFile jarFile = new JarFile(url.getPath())) {
+ Enumeration<JarEntry> entries = jarFile.entries();
+
+ while (entries.hasMoreElements()) {
+ JarEntry entry = entries.nextElement();
+ String entryName = entry.getName();
+
+ if (!entry.isDirectory() &&
EXTENSION_DEPENDENCIES_JSON.equals(entryName)) {
+ log.info("Found extension dependency entry in druid jar %s",
url.getPath());
Review Comment:
To reduce logging chatter, make this `log.debug` and add the dependency info
to the `Loading extension [%s], jars: %s` message.
##########
processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java:
##########
@@ -290,13 +302,44 @@ private void addAllFromCurrentClassLoader()
private void addAllFromFileSystem()
{
+ Map<String, URLClassLoader> extensionClassLoaderMap = new HashMap<>();
+
+ for (File extension : getExtensionFilesToLoad()) {
+ extensionClassLoaderMap.put(
+ extension.getName(),
+ getClassLoaderForExtension(
+ extension,
+ extensionsConfig.isUseExtensionClassloaderFirst()
+ )
+ );
+ }
+
for (File extension : getExtensionFilesToLoad()) {
log.debug("Loading extension [%s] for class [%s]",
extension.getName(), serviceClass);
try {
final URLClassLoader loader = getClassLoaderForExtension(
extension,
extensionsConfig.isUseExtensionClassloaderFirst()
);
+ Optional<ExtensionDependencies> extensionDependencies =
getExtensionDependencies(loader);
+ List<String> extensionDruidExtensionDependencies =
extensionDependencies.isPresent()
+ ? extensionDependencies.get().getDependsOnDruidExtensions()
+ : ImmutableList.of();
+
+ List<ClassLoader> chainedClassLoadersForExtension = new
ArrayList<>();
+ for (String druidExtensionDependency :
extensionDruidExtensionDependencies) {
+ if
(!extensionClassLoaderMap.containsKey(druidExtensionDependency)) {
+ throw new RE(
+ StringUtils.format(
+ "%s depends on %s which is not a valid extension or not
loaded.",
Review Comment:
House style for error messages includes square brackets around `%s`, with no
whitespace before the brackets. So something like `Extension[%s] depends on
extension[%s], which is not present`
--
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]
