inter-Extension dependency support by georgew5656 · Pull Request #16973 · apache/druid

georgew5656 · 2024-08-28T20:48:46Z

Allow specifying extensions between extensions without having to explicitly install jars in multiple places.

Description

Currently, in order to have extension A depend on extension B, you have to explicitly include extension B as a build dependency in extension A's pom.xml. This cause the jar for extension B to be included in extension A's directory and loaded as a module.

This can lead to some weird behavior e.g. the following bug linked from #16929 caused by druid-kafka-extraction-namespace depending on druid-lookups-cached-global.

There is logic in ExtensionsLoader.tryAdd that checks whether a module has already been loaded during initialization and skips it if it already has been loaded.

This is a problem when both druid-kafka-extraction-namespace and druid-lookups-cached-global are specified because they both load NamespaceExtractionModule.

If druid-kafka-extraction-namespace is specified first, both NamespaceExtractionModule and KafkaExtractionNamespaceModule are loaded by the druid-kafka-extraction-namespace classloader, and the druid-lookups-cached-global classloader doesn't load anything since NamespaceExtractionModule was already loaded. This is fine because the features of druid-lookups-cached-global are served through the module NamespaceExtractionModule being loaded in druid-kafka-extraction-namespace. (this is essentially the same behavior as just loading druid-kafka-extraction-namespace, and this is why loading both extensions in this order works)

If druid-lookups-cached-global is specified first, NamespaceExtractionModule is loaded by the druid-lookups-cached-global class loader. The druid-kafka-extraction-namespace classloader will only load KafkaExtractionNamespaceModule because NamespaceExtractionModule has already been loaded. This is a problem because kafka lookups rely on classes bound in NamespaceExtractionModule that it can't access (because NamespaceExtractionModule is only bound in the druid-lookups-cached-global classloader).

to get around this issue, my proposed fix is to allow chaining classloaders without having to actually copy jars. e.g. extension A won't actually have extension B's jar (we include the dependency as provided in the pom.xml). when extension A is loading classes, it will try to use extension B's classloader to find classes it can't find.

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Classloaders
We currently support two kinds of classloaders for each extension (extension-first or not). hadoop always uses the non extension-first classloader and other druid services check the druid.extensions.useExtensionClassloaderFirst property.

I made a assumption to simplify the change in addAllFromFileSystem() (the code in ExtensionsLoader where chained classloaders are set up). in this code we always use the classloader based off of druid.extensions.useExtensionClassloaderFirst, so if druid.extensions.useExtensionClassloaderFirst=true, the StandardClassLoader objects won't get chained classloading setup.

It seems like this could be a issue with this line in HadoopTask (https://github.com/apache/druid/blob/master/indexing-service/src/main/java/org/apache/druid/indexing/common/task/HadoopTask.java#L158), because if druid.extensions.useExtensionClassloaderFirst=true is set, only the ExtensionFirstClassLoaders will have chained classloading setup, but it seems like all HadoopTask does is combine all the resource urls from all the extensions so I don't think this is a issue. plus this is the current behavior.

this made it a little more annoying to implement chained classloading (see ExtensionLoader), but I didn't think we could deprecate this behavior so I left it in. I had to add a StandardClassLoader class because we were just using a regular UrlClassLoader object for the non extension-first classloader. In both StandardClassLoader and ExtensionFirstClassLoader i added the logic to check other extension's classloaders for classes. this is the part of the code i'd like some more detailed feedback on.

Extension dependency specification
I added a extension-dependencies.json per-extension resources file to specify inter-extension dependencies (see extensions-core/kafka-extraction-namespace/src/main/resources/extension-dependencies.json for a example)

When loading extensions, ExtensionsLoader looks for a druid-* jar (the main extension code), and inspects it for a extension-dependencies.json resource file. If this file exists, it injects the correct chained classloaders to the extension classloader.

I tested this with druid-kafka-extraction-namespace and druid-lookups-cached-global and everything seems to work okay.

I haven't added any unit tests yet because I want to get feedback on whether this approach seems reasonable.

Release note

support explicit extension dependencies in druid

Key changed/added classes in this PR

ExtensionLoader
ExtensionFirstClassLoader
StandardClassLoader
PullDependencies

This PR has:

…rdering

suneet-s

Very nice! I'm not best positioned to actually review the change, but it would be good to add this to the instructions how to build an extension.

Closest thing I found to something like this was

druid/docs/development/modules.md

Line 371 in aa833a7

### Managing dependencies

gianm · 2024-09-11T15:45:16Z

+        );
+      }
+
      for (File extension : getExtensionFilesToLoad()) {


It would be cleaner to iterate extensionClassLoaderMap.entrySet() here so we don't need to re-read the extensions directory and re-call getClassLoaderForExtension.

gianm · 2024-09-11T15:46:55Z

+    {
+      for (URL url : loader.getURLs()) {
+        File jarFileLocation = new File(url.getPath());
+        if (jarFileLocation.getName().startsWith("druid")) {


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.

gianm · 2024-09-11T15:47:34Z

 {
  private static final Logger log = new Logger(ExtensionsLoader.class);
-
+  public static final String EXTENSION_DEPENDENCIES_JSON = "extension-dependencies.json";


Should have druid somewhere in the name, like druid-extension-dependencies.json. It should also be in resources/ inside the jar.

i think the resources files get packaged into the root level of the jar, at least thats what i saw from unpacking the jar.

so in this case druid-extension-dependencies.json would be directly unzipped from the jar, not under a resources/ directory. i updated the name though

gianm · 2024-09-11T15:48:55Z

+              String entryName = entry.getName();
+
+              if (!entry.isDirectory() && EXTENSION_DEPENDENCIES_JSON.equals(entryName)) {
+                log.info("Found extension dependency entry in druid jar %s", url.getPath());


To reduce logging chatter, make this log.debug and add the dependency info to the Loading extension [%s], jars: %s message.

gianm · 2024-09-11T15:51:40Z

+            if (!extensionClassLoaderMap.containsKey(druidExtensionDependency)) {
+              throw new RE(
+                  StringUtils.format(
+                      "%s depends on %s which is not a valid extension or not loaded.",


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

gianm · 2024-09-11T15:56:43Z

+            }
+            chainedClassLoadersForExtension.add(extensionClassLoaderMap.get(druidExtensionDependency));
+          }
+          ((StandardClassLoader) loader).setExtensionDependencyClassLoaders(chainedClassLoadersForExtension);


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.)

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.

…ependencySupport

gianm · 2024-09-24T05:54:41Z

+          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());


Just "in jar" suffices, since we aren't filtering down to specifically Druid jars anymore.

House style for error messages would include brackets around %s.

gianm · 2024-09-24T05:56:02Z

+            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.",


Same here, "multiple jars" is better than "multiple druid jars". Also, list the jars? Otherwise the error message is going to be potentially confusing.

gianm · 2024-09-24T05:56:45Z

+        }
+      }
+      catch (IOException e) {
+        throw new RuntimeException(e);


Include the extension path in the error message, like throw new RE(e, "Failed to get dependencies for extension[%s]", extension);

gianm · 2024-09-24T05:57:44Z

+        extensionDependencyStack.add(extensionDependencyFile.getName());
+        throw new RE(
+            StringUtils.format(
+                "[%s] has a circular druid extension dependency. Dependency stack [%s].",


Extension[%s] is better than just [%s].

gianm · 2024-09-24T05:57:47Z

+      if (!extensionDependencyFileOptional.isPresent()) {
+        throw new RE(
+            StringUtils.format(
+                "[%s] depends on [%s] which is not a valid extension or not loaded.",


Extension[%s] is better than just [%s].

gianm · 2024-09-24T06:00:37Z

+    return getClassLoaderForExtension(extension, useExtensionClassloaderFirst, new ArrayList<>());
+  }
+
+  public StandardURLClassLoader getClassLoaderForExtension(File extension, boolean useExtensionClassloaderFirst, List<String> extensionDependencyStack)


Javadoc please, explaining what extensionDependencyStack is and how it's used. It isn't obvious from the context.

gianm · 2024-09-24T06:03:51Z

  private final ConcurrentHashMap<Class<?>, Collection<?>> extensions = new ConcurrentHashMap<>();

+  @MonotonicNonNull
+  private File[] extensionFilesToLoad;


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.


  @VisibleForTesting
-  public Map<Pair<File, Boolean>, URLClassLoader> getLoadersMap()
+  public Map<Pair<File, Boolean>, StandardURLClassLoader> getLoadersMap()


gianm

LGTM, thanks for the change!

georgew5656 added 8 commits August 20, 2024 15:50

update docs for kafka lookup extension to specify correct extension o…

09c0898

…rdering

fix first line

cfa23fa

test with extension dependencies

2ccaf61

save work on dependency management

5980b70

working dependency graph

95e3803

working pull

3f94868

fix style

5a5802d

fix style

406f2e6

georgew5656 mentioned this pull request Aug 28, 2024

(WIP) Support explicit dependencies between extensions. #16968

Closed

10 tasks

github-actions Bot added Area - Lookups Area - Dependencies labels Aug 28, 2024

remove name

e30c6bd

georgew5656 requested review from abhishekagarwal87, abhishekrb19, gianm, kfaraz and suneet-s September 9, 2024 13:52

suneet-s reviewed Sep 9, 2024

View reviewed changes

gianm reviewed Sep 11, 2024

View reviewed changes

georgew5656 added 4 commits September 11, 2024 15:42

load extension dependencies recursively

536d72f

generate depenencies on classloader creation

c670eff

add check for circular dependencies

86286fd

fix style

60eff31

github-actions Bot added the Area - Ingestion label Sep 12, 2024

georgew5656 added 3 commits September 12, 2024 15:06

revert style changes

94eea81

remove mutable class loader

5cc471d

clean up class heirarchy

4eea721

github-advanced-security AI found potential problems Sep 12, 2024

View reviewed changes

Comment thread processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java Fixed

georgew5656 added 2 commits September 13, 2024 10:42

extensions loader test working

8b77d6a

add unit tests

5098bda

georgew5656 requested a review from gianm September 13, 2024 18:42

Merge branch 'master' of github.com:georgew5656/druid into extensionD…

1feb327

…ependencySupport

gianm reviewed Sep 24, 2024

View reviewed changes

pr comments

37b2990

georgew5656 requested a review from gianm September 24, 2024 14:41

github-advanced-security AI found potential problems Sep 24, 2024

View reviewed changes

fix unit tests

10c4d80

gianm approved these changes Sep 24, 2024

View reviewed changes

georgew5656 merged commit d1bfabb into apache:master Sep 24, 2024

abhishekagarwal87 mentioned this pull request Oct 10, 2024

Track 3rd party libs used in the dist package #17208

Open

adarshsanjeev added this to the 32.0.0 milestone Jan 16, 2025

adarshsanjeev mentioned this pull request Jan 28, 2025

[DRAFT] 32.0.0 release notes #17677

Closed

Conversation

georgew5656 commented Aug 28, 2024

Description

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Release note

Key changed/added classes in this PR

Uh oh!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check notice

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants