Move x-content implementation to a separate classloader #83705

rjernst · 2022-02-09T03:32:15Z

x-content is a relatively well contained abstraction for parsing and generating json-like content. It is backed by Jackson, but consumers throughout Elasticsearch use the x-content interfaces and don't generally care about how Jackson is used. However, because x-content is a dependency of server, Jackson is also a dependency of server. Since all server dependencies must be exactly matched by plugins (and ES modules), upgrading Jackson is difficult since it must be done in lock step across the codebase.

This change isolates the Jackson implementation of x-content parsers and generators to a separate classloader. The code is loaded dynamically upon accessing any x-content functionality.

The x-content implementation is embedded inside the x-content jar, as a hidden set of resource files. These are loaded through a special classloader created to initialize the XContentProvider through service loader. One caveat to this approach is that IDEs will no longer trigger building the x-content implementation when it changes. However, running any test from the command line, or running a full Build in IntelliJ will trigger the directory to be built.

Most of the changes here are logistical, just moving methods and classes across to the implementation project. One common change to code using x-content is exception handling. This code relied on knowledge of Jackson exception types. That code now uses special exception types from x-content. Another change needed in a few places was now including jackson-core in plugins, since server no longer provides it.

The hope is that this change can be used as a model for future similar isolation of server dependencies.

Also delete duplicated tests, keeping them in x-content lib

…, 73 skipped

Incremental progress - X-Content-split

…ved :-(

X content split - iteration 2

rjernst · 2022-02-14T16:56:07Z

@mark-vieira @nik9000 I think this is ready for another round of review. The implementation has changed so that the implementation jars are now exploded and embedded in the x-content jar in a hidden location. This alleviates the need to add anything additional to the classpath, or have any new artifacts published.

@nik9000 Thanks for looking at Eclipse. It's about exactly what we anticipated. I'm not sure we should do anything. The hack you came up with is quite fragile and long term Eclipse is going to have more of these problems as we move to modules. An alternative hack would be to put the genereated resources directory on the classpath manually within the project. It's not much worse than what IntelliJ is doing automatically by reading the gradle model, but it still requires running any cli command that will trigger building the directory. I know it isn't what you want to hear, but I think the benefit of this change outweighs complexity added for the few developers using Eclipse.

mark-vieira

This works well and LGTM other than a few comments. Thanks for iterating on this. I think this implementation is definitely better and more sustainable going forward.

mark-vieira · 2022-02-14T21:51:41Z

libs/x-content/impl/build.gradle

+ */
+
+apply plugin: 'elasticsearch.build'
+apply plugin: 'elasticsearch.publish'


Do we still need to publish this given we are effectively embedding it in the x-content jar?

mark-vieira · 2022-02-14T21:53:15Z

libs/x-content/impl/build.gradle

+}
+
+tasks.named("jar").configure {
+  archiveBaseName = "x-content-impl"


We should just set this on the project convention. That is, add archivesBaseName = "x-content-impl" to the root of the build script

mark-vieira · 2022-02-14T21:54:25Z

libs/x-content/impl/build.gradle

+ * Side Public License, v 1.
+ */
+
+apply plugin: 'elasticsearch.build'


I think we want to use the elasticsearch.java plugin here since build implicitly applies the publish plugin which I don't believe we want.

mark-vieira · 2022-02-14T21:56:12Z

libs/x-content/build.gradle

@@ -47,3 +60,24 @@ tasks.named("dependencyLicenses").configure {
 }

 tasks.named("jarHell").configure { enabled = false }


Why are we disabling this?

mark-vieira · 2022-02-14T21:59:57Z

libs/x-content/build.gradle

+
+File generatedResourcesDir = new File(buildDir, 'generated-resources')
+def generateProviderManifest = tasks.register("generateProviderManifest") {
+  File manifestFile = new File(generatedResourcesDir, "MANIFEST.TXT")


I feel like this should have a more descriptive name. Also confusing given we'll have a MANIFEST.TXT and MANIFEST.MF in the JAR.

I actually named it manifest since it sort of parallels the jar manifest. I'm not tied to the name, though. I just don't have a better one. Do you have a suggestion?

Something with "embedded" in the name?

I pushed "LISTING.TXT", wdyt?

Works for me.

libs/x-content/src/main/java/org/elasticsearch/xcontent/internal/EmbeddedImplClassLoader.java

libs/x-content/build.gradle

breskeby · 2022-02-15T16:56:23Z

We should apply a custom attribute for the providerImpl configuration and on the registered transformation to
ensure we only apply the transformation for dependencies resolved from the providerImpl configuration.

E.g.

configurations {
  providerImpl {
    canBeConsumed = false
    attributes.attribute(isProviderImpl, Boolean.TRUE)
  }
}

...
...

dependencies {
  registerTransform(
    UnzipTransform.class, transformSpec -> {
    transformSpec.getFrom()
            .attribute(ArtifactTypeDefinition.ARTIFACT_TYPE_ATTRIBUTE, ArtifactTypeDefinition.JAR_TYPE)
            .attribute(isProviderImpl, true)

    transformSpec.getTo()
            .attribute(ArtifactTypeDefinition.ARTIFACT_TYPE_ATTRIBUTE, ArtifactTypeDefinition.DIRECTORY_TYPE)
            .attribute(isProviderImpl, true)

    transformSpec.parameters(parameters -> {
      parameters.includeArtifactName.set(true)
    })

  })

...
}

This still results in the original raised circular dependency issue but with a clearer output:

Circular dependency between the following tasks:
:libs:elasticsearch-x-content:classes
\--- :libs:elasticsearch-x-content:generateProviderImpl
     +--- :libs:elasticsearch-x-content:generateProviderManifest
     |    \--- :libs:elasticsearch-x-content:impl:jar
     |         \--- :libs:elasticsearch-x-content:impl:classes
     |              \--- :libs:elasticsearch-x-content:impl:compileJava
     |                   \--- :libs:elasticsearch-x-content:jar
     |                        +--- :libs:elasticsearch-x-content:classes (*)
     |                        \--- :libs:elasticsearch-x-content:generateProviderImpl (*)

The root problem keeps being that running classes triggers the generateProviderImpl as we define
sourceSets.main.output.dir(generateProviderImpl).
This requires impl to be build which causes the circular dependency as impl has a compile dependency on x-content itself.

rjernst · 2022-02-15T18:16:13Z

@breskeby The circular issue was on a different branch, just one that now includes the x-content separation from this PR. This PR builds find and has passed CI. I pushed 5672c22, let me know if that matches what you were thinking.

breskeby · 2022-02-16T11:07:37Z

thanks @rjernst. looks good to me

breskeby

lgtm

x-content has support for automatically detecting the content type provided, through a series of heuristics for each supported x-content type. This change moves those heuristics into the relevant x-content implementation classes so that XContentFactory does not have direct dependencies on those Jackson classes. relates elastic#83705

This change isolates the Jackson implementation of x-content parsers and generators to a separate classloader. The code is loaded dynamically upon accessing any x-content functionality. The x-content implementation is embedded inside the x-content jar, as a hidden set of resource files. These are loaded through a special classloader created to initialize the XContentProvider through service loader. One caveat to this approach is that IDEs will no longer trigger building the x-content implementation when it changes. However, running any test from the command line, or running a full Build in IntelliJ will trigger the directory to be built. Co-authored-by: ChrisHegarty <christopher.hegarty@elastic.co>

ChrisHegarty and others added 30 commits February 3, 2022 14:16

X-Content Provider prototype

2f6a487

Move content detection into XContent

7c00dde

Also delete duplicated tests, keeping them in x-content lib

Replace most parsing exceptions with XContentParseException

3eb5b6a

Unused import

f0b37ae

Add JsonStringEncoder SPI

0d8147a

License / notice updates

684fc81

all tests compile

d9d6f1b

rename from o.e.xc.internal to o.e.xc.provider

f411ab1

Add XContentGenerationException and update test

d42023c

D'oh!

8a4511d

Move the x content provider locator to its own "internal" class

7d740e0

Random small changes - server tests: 13122 tests completed, 12 failed…

3ffbea7

…, 73 skipped

checkstyle and forbidden

ec3a6d4

license, notice, sha

4b43088

thirdPartyAudit ignore missing classes x-content-provider

444801d

most x-content tests passing: 408 tests completed, 1 failed, 8 skipped

fff33bb

all x-content tests passing

42ac1c2

Merge pull request #1 from ChrisHegarty/x-content-split

3a88d04

Incremental progress - X-Content-split

cleanup commented code

c0de72e

First attempt at embedding impl jars

9ec7706

add provider jars as directory on classpath

749e603

tweak provider jars dir on the classpath

7a76da4

update shas

be7e507

style

6482b8c

launchers need snakeyaml directly - update license

a84bf39

unused jackson import in ml

e936d78

explicitly disable transitive dependencies - databind was being resol…

fbba9cf

…ved :-(

Merge pull request #2 from ChrisHegarty/x-content-split

554e73b

X content split - iteration 2

fix distribution to run with provider impl directory

7b65603

add back manifest file as generated source dir

90ae342

rjernst and others added 11 commits February 11, 2022 21:37

fix oopses

56d1709

checkstyle

3c1cb8f

licenses

285b28c

javadoc

7c3efc3

fix inner class loading

15ac750

enumeration of enumerations

6e5fe03

move EmbeddedImplClassLoader to its own top-level file

3706212

EmbeddedImplClassLoader cleanup

cc78efe

OBOE

c54f7b3

x-content specific structure

bc36311

minimal javadoc

846a41b

ChrisHegarty added 2 commits February 14, 2022 19:45

add codesource

c7e5d12

map to codesource (not URL)

46c87e3

mark-vieira approved these changes Feb 14, 2022

View reviewed changes

rjernst added 4 commits February 14, 2022 14:50

reenable jarhell

7aa3668

don't publish impl

84b2fd2

change manifest file name to "listing"

761cae4

add back deleted test

9a2e5a5

add impl attribute to restrict artifact transform

5672c22

breskeby approved these changes Feb 16, 2022

View reviewed changes

fix embedded impl code source

30c4a16

rjernst merged commit 070fcaa into elastic:master Mar 7, 2022

rjernst deleted the x-content-split branch March 7, 2022 23:45

d2a-pnagel mentioned this pull request Feb 20, 2023

Jetty logs warning with stacktrace when annotation parser encounters module-info.class file inside elasticsearch-x-content jar jetty/jetty.project#9400

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move x-content implementation to a separate classloader #83705

Move x-content implementation to a separate classloader #83705

rjernst commented Feb 9, 2022 •

edited

rjernst commented Feb 14, 2022

mark-vieira left a comment

mark-vieira Feb 14, 2022

mark-vieira Feb 14, 2022

mark-vieira Feb 14, 2022

mark-vieira Feb 14, 2022

mark-vieira Feb 14, 2022

rjernst Feb 14, 2022

mark-vieira Feb 14, 2022

rjernst Feb 14, 2022

mark-vieira Feb 14, 2022

breskeby commented Feb 15, 2022

rjernst commented Feb 15, 2022

breskeby commented Feb 16, 2022

breskeby left a comment

		@@ -47,3 +60,24 @@ tasks.named("dependencyLicenses").configure {
		}

		tasks.named("jarHell").configure { enabled = false }

Move x-content implementation to a separate classloader #83705

Move x-content implementation to a separate classloader #83705

Conversation

rjernst commented Feb 9, 2022 • edited

rjernst commented Feb 14, 2022

mark-vieira left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breskeby commented Feb 15, 2022

rjernst commented Feb 15, 2022

breskeby commented Feb 16, 2022

breskeby left a comment

Choose a reason for hiding this comment

rjernst commented Feb 9, 2022 •

edited