Introduce Plexus ClassWorld for more control over the classloaders #13845

rfscholte · 2024-08-19T13:07:26Z

There is a known and longstanding issue with pinot plugins and class colissions. The problem lies in not being able to control where classes should be coming from. In the end it is all about classloaders.
The JRE recognized this issue and came with the Java Platform Module System, but that comes with other requirements.
Apache Maven also has the concept of plugins and has been using Plexus Classworld to solve that issue (see https://maven.apache.org/guides/mini/guide-maven-classloading.html)
With Plexus Classworld it is possible to define the order of classloaders, scope the exported and imported packages, etc. , see https://codehaus-plexus.github.io/plexus-classworlds/ .
This PR will introduce a library that has proven itself. Even if discover other classloading issues, it will possible to configure Plexus Classworld to solve that.

…loaders

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java

…here should be different configuration for this instance

codecov-commenter · 2024-08-19T16:06:59Z

Codecov Report

Attention: Patch coverage is 29.41176% with 48 lines in your changes missing coverage. Please review.

Project coverage is 57.56%. Comparing base (59551e4) to head (440db9d).
Report is 931 commits behind head on master.

Files	Patch %	Lines
...ava/org/apache/pinot/spi/plugin/PluginManager.java	35.71%	31 Missing and 5 partials ⚠️
...che/pinot/spi/plugin/PinotPluginConfiguration.java	0.00%	12 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #13845      +/-   ##
============================================
- Coverage     61.75%   57.56%   -4.19%     
+ Complexity      207      197      -10     
============================================
  Files          2436     2578     +142     
  Lines        133233   142305    +9072     
  Branches      20636    22102    +1466     
============================================
- Hits          82274    81914     -360     
- Misses        44911    53899    +8988     
- Partials       6048     6492     +444

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`57.51% <29.41%> (-4.20%)`	⬇️
java-21	`57.44% <29.41%> (-4.18%)`	⬇️
skip-bytebuffers-false	`57.53% <29.41%> (-4.21%)`	⬇️
skip-bytebuffers-true	`57.42% <29.41%> (+29.69%)`	⬆️
temurin	`57.56% <29.41%> (-4.19%)`	⬇️
unittests	`57.55% <29.41%> (-4.19%)`	⬇️
unittests1	`40.27% <29.41%> (-6.62%)`	⬇️
unittests2	`27.84% <0.00%> (+0.11%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pinot-spi/pom.xml

Jackie-Jiang

Are jar files included intentionally? It will add almost 40MB to the repo

rfscholte · 2024-08-20T07:08:16Z

Are jar files included intentionally? It will add almost 40MB to the repo

It can be replaced with any jar, but that means it will be a customized jar. What I can do is remove all the unused (untested) classes from this jar. Or do you have something else in mind?

gortiz · 2024-08-20T08:23:38Z

Question about the Jars: If we change the test to be an integration test and we use failsafe instead of surfire... would we be able to consume the actual jar from the plugins instead of the ones we include in resources?

rfscholte · 2024-08-20T12:09:11Z

I guess the question is: can we let Maven download the jars instead? In this case the jars are test-resources and not dependencies, so I wouldn't abuse test-scoped dependencies. What we can do is to make use of https://maven.apache.org/plugins/maven-dependency-plugin/copy-mojo.html to copy the dependencies to the preferred location. Let me prepare that. (in this case it is still a unittest, so no benefits when using failsafe)

rfscholte · 2024-08-20T13:28:21Z

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java

+        ClassLoader pinotLoader = _classWorld.getClassRealm("pinot");
+
+        // All exported packages
+        Stream.of("org.apache.pinot.spi.accounting",


This list is an assumption. Plugins should only make use of classes in these packages, which is any inside the pinot-spi module. Any other pinot-packages will not be accessible.

rfscholte · 2024-08-20T13:30:10Z

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java

@@ -41,11 +47,16 @@ public class PluginManager {

  public static final String PLUGINS_DIR_PROPERTY_NAME = "plugins.dir";
  public static final String PLUGINS_INCLUDE_PROPERTY_NAME = "plugins.include";
+  public static final String PLUGINS_LOADER_LEGACY_PROPERTY_NAME = "pinot.plugins.loader.legacy";


I've introduced a system property to fall back to the original behavior. I can imagine that some plugins rely on internal pinot-classes, which will now fail.

Jackie-Jiang · 2024-08-20T18:07:00Z

Could you please update the PR description with some context to help people understand the intention of the PR?

gortiz · 2024-08-21T14:09:57Z

pinot-spi/pom.xml

+          <execution>
+            <id>copy-pinot-plugins</id>
+            <phase>process-test-resources</phase>
+            <goals>
+              <goal>copy</goal>
+            </goals>


nit: I think this is a good idea but probably devs are not going to understand on a first read why this plugin is necessary.

Can you add a comment here saying that PluginManagerTest requires these resources and we decided to copy them at build time?

Agreed, this could use an extra comment

pinot-spi/pom.xml

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java

gortiz · 2024-08-21T14:18:04Z

PluginManagerTest test is failing right now with:

java.lang.RuntimeException: org.codehaus.plexus.classworlds.realm.NoSuchRealmException: pinot-dropwizard
	at org.apache.pinot.spi.plugin.PluginManager.loadClass(PluginManager.java:380)
	at org.apache.pinot.spi.plugin.PluginManagerTest.classRealms(PluginManagerTest.java:217)

…st-classes/plugins

…ot you'll have "unlimited" mode, all classes will be looked up in the pinot-realm first.

…ault parent

gortiz · 2024-08-29T09:12:18Z

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java

@@ -41,10 +51,18 @@ public class PluginManager {

  public static final String PLUGINS_DIR_PROPERTY_NAME = "plugins.dir";
  public static final String PLUGINS_INCLUDE_PROPERTY_NAME = "plugins.include";
+  public static final String PLUGINS_LOADER_LEGACY_PROPERTY_NAME = "pinot.plugins.loader.legacy";
+  public static final String PLUGINS_LOADER_LEGACY_DEFAULT = "false";


This should be true for now.

Probably we should rename the property to be something like pinot.plugins.loader=plexus. In case the property is not defined, we use the legacy mode. In the future we will be able to change the default to plexus.

If we make legacy the default, I wonder if anyone dares to switch to plexus classworld, even though classloader issues keep happening. Instead I propose to make plexus classworlds behave more like its current behavior. i.e. having access to all classes (like now) but loading them in the expected order. In other words: make them unlimited. The switch to limited as default is smaller.

We are used to use flags to enable new features to be sure we do not affect deployments where the flag is not set. I'm sure StarTree will use the new system in their deployments (after testing it for a while), so I'm sure it is going to be used.

gortiz · 2024-09-03T08:07:22Z

I'm going to close this PR because Robert has been focused on #13907, which also includes the new plugin layout and simplifies the logic here

STP-3362: Introduce Plexus ClassWorld for more control over the class…

d94887e

…loaders

gortiz reviewed Aug 19, 2024

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java Show resolved Hide resolved

rfscholte added 3 commits August 19, 2024 15:37

Fix checkstyle reported issues

51a9c24

Add plexus-classworlds dependency to pom

81ad98b

Cannot use PluginManager.get() as this returns a singleton, whereas t…

b8b430d

…here should be different configuration for this instance

gortiz reviewed Aug 19, 2024

View reviewed changes

pinot-spi/pom.xml Outdated Show resolved Hide resolved

Specify version of classworlds org.apache.pinot:pinot

36b2245

Jackie-Jiang reviewed Aug 19, 2024

View reviewed changes

Jackie-Jiang added enhancement dependencies Pull requests that update a dependency file labels Aug 19, 2024

Let Maven download the jars

e629621

rfscholte commented Aug 20, 2024

View reviewed changes

gortiz reviewed Aug 21, 2024

View reviewed changes

pinot-spi/pom.xml Show resolved Hide resolved

gortiz reviewed Aug 21, 2024

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java Show resolved Hide resolved

rfscholte added 8 commits August 21, 2024 17:57

Fix unittest: dependency-plugin copies pinot-plugin jars to target/te…

a3058b1

…st-classes/plugins

Explain usage of maven-dependency-plugin

cf191be

Add logmessage to show which classloader is being used

3ecb7d2

Merge branch 'apache:master' into STP-3362_classrealms

293bf56

Add ability to let plugins define the importForm per realm by itself.

5b55959

Improve tests: ensure commonsIOUtils is always loaded from its own realm

440db9d

By adding a pinot-plugin.properties with the entry parent.realmId=pin…

d6add33

…ot you'll have "unlimited" mode, all classes will be looked up in the pinot-realm first.

Proper use of parent and baseclassloader

cf173db

rfscholte added 2 commits August 29, 2024 09:00

Simplify the code by using pinot realm with predefined imports as def…

6f7afa8

…ault parent

Improve code related to classrealms

157e871

gortiz reviewed Aug 29, 2024

View reviewed changes

Merge branch 'master' into STP-3362_classrealms

cf7e189

rfscholte mentioned this pull request Aug 29, 2024

pinot plugin loader #13907

Closed

Merge branch 'apache:master' into STP-3362_classrealms

fa30ac4

gortiz closed this Sep 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Plexus ClassWorld for more control over the classloaders #13845

Introduce Plexus ClassWorld for more control over the classloaders #13845

rfscholte commented Aug 19, 2024 •

edited by Jackie-Jiang

Loading

codecov-commenter commented Aug 19, 2024 •

edited

Loading

Jackie-Jiang left a comment

rfscholte commented Aug 20, 2024

gortiz commented Aug 20, 2024

rfscholte commented Aug 20, 2024

rfscholte Aug 20, 2024

rfscholte Aug 20, 2024

Jackie-Jiang commented Aug 20, 2024

gortiz Aug 21, 2024

rfscholte Aug 21, 2024

gortiz commented Aug 21, 2024

gortiz Aug 29, 2024

gortiz Aug 29, 2024

rfscholte Aug 29, 2024

gortiz Aug 29, 2024

gortiz commented Sep 3, 2024

Introduce Plexus ClassWorld for more control over the classloaders #13845

Introduce Plexus ClassWorld for more control over the classloaders #13845

Conversation

rfscholte commented Aug 19, 2024 • edited by Jackie-Jiang Loading

codecov-commenter commented Aug 19, 2024 • edited Loading

Codecov Report

Jackie-Jiang left a comment

Choose a reason for hiding this comment

rfscholte commented Aug 20, 2024

gortiz commented Aug 20, 2024

rfscholte commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackie-Jiang commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gortiz commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gortiz commented Sep 3, 2024

rfscholte commented Aug 19, 2024 •

edited by Jackie-Jiang

Loading

codecov-commenter commented Aug 19, 2024 •

edited

Loading