Make x-pack core pull transport-nio by Tim-Brooks · Pull Request #32757 · elastic/elasticsearch

Tim-Brooks · 2018-08-09T19:53:35Z

The security nio transports depend on transport-nio. This commit
modifies x-pack core to include the transport-nio jar into the x-pack
core module.

The security nio transports depend on transport-nio. This commit modifies x-pack core to include the transport-nio jar into the x-pack core module.

elasticmachine · 2018-08-09T19:53:37Z

Pinging @elastic/es-security

Tim-Brooks · 2018-08-09T19:53:55Z

@danielmitterdorfer - this will fix the issue you were encountering.

rjernst

LGTM

rjernst · 2018-08-09T21:09:11Z

x-pack/plugin/core/build.gradle

    shadow 'com.unboundid:unboundid-ldapsdk:3.2.0'
    shadow project(path: ':modules:transport-netty4', configuration: 'runtime')
+    shadow(project(path: ':plugins:transport-nio', configuration: 'runtime')) {
+        exclude group: "org.elasticsearch", module: "elasticsearch-core"


This looks like a bug in esplugin, that core is not being marked as compileOnly in this case (yet it is for depending on the transport-netty4 module). @atorok can you investigate this and open an issue? @tbrooks8 can you add a TODO/comment here that this should not be necessary

alpar-t · 2018-08-10T05:31:41Z

x-pack/plugin/core/build.gradle

+    shadow(project(path: ':plugins:transport-nio', configuration: 'runtime')) {
+        // TODO: This should not be necessary if core is compile only
+        exclude group: "org.elasticsearch", module: "elasticsearch-core"
+        exclude group: "io.netty", module: "netty-buffer"


We disable transitive dependencies for anything but org.elasticsearch so these excludes shouldn't be required either.

transport-nio is under org.elasticsearch, that is why it gets transitive deps.

alpar-t · 2018-08-10T06:47:45Z

For clarity, the exclude is required to avoid jar hell

|    Caused by: java.lang.IllegalStateException: jar hell!
|    class: META-INF.versions.9.org.elasticsearch.core.internal.io.Streams
|    jar1: /home/alpar/work/elastic/elasticsearch/x-pack/plugin/build/cluster/integTestCluster node0/elasticsearch-7.0.0-alpha1-SNAPSHOT/modules/x-pack-core/elasticsearch-core-7.0.0-alpha1-SNAPSHOT.jar
|    jar2: /home/alpar/work/elastic/elasticsearch/x-pack/plugin/build/cluster/integTestCluster node0/elasticsearch-7.0.0-alpha1-SNAPSHOT/lib/elasticsearch-core-7.0.0-alpha1-SNAPSHOT.jar

:plugins:transport-nio pulls in libs:nio via the runtime configruation, which pulls :libs:core according tot his scan.

transport-netty4 does not have dependencies to any other libs, like transport-nio depends on :libs:core thus it won't have this problem.

The problem is that :plugins:transport-nio has a compile dependency on :libs:core, but this one is already a dependency of the server, so will be provided when the plugin loads, thus it should be a compileOnly dependency. As a consequence of this, project that depend on :plugins:transport-nio, like :x-pack:plugins:security will have to have a compileOnly dependency on :libs:nio or any other transitive dependency of the former for that matter as compileOnly doesn't carry transitive dependencies.

@rjernst
As a side note, while investigating this (and others in the past) I'm constantly puzzled and confused about indirection like this org.elasticsearch:elasticsearch-nio:7.0.0-alpha1-SNAPSHOT. I'm wondering if this is just me ? Why was it done like this ? Should we clean up and always use project dependencies to refer to projects. The example plugins are the only place where I think this makes sense, but we could do that locally.

@tbrooks8 I made the changes mentioned above in atorok@4dd5ea3d18e

Tim-Brooks · 2018-08-10T16:14:32Z

@rjernst @atorok - I took @atorok changes. Thanks! Can someone take a look and confirm that everything looks okay?

rjernst · 2018-08-10T16:50:16Z

Thanks for looking at this @atorok. IMO it does not make sense to have core as a compileOnly dep of libs:nio. It is confusing for downstream consumers to have to redeclare that dependency. Even if both server and nio here depend on core, it should not cause jarhell, because there should only be one copy. Xpack core copying other plugins (transport netty and nio) is a special case that we should not design for. This is not something we normally do, so other dep lists should not be tweaked to make that case easier.

rjernst · 2018-08-10T16:56:12Z

I'm constantly puzzled and confused about indirection like this org.elasticsearch:elasticsearch-nio:7.0.0-alpha1-SNAPSHOT

This is mostly a bit of confusion on my part in the original gradle implementation. I had thought at the time that project dependencies would not show up in generated poms. Note, though, that we do use project substitutions for other reasons, for example within buildSrc, where we cannot depend on these being projects. Personally I have come to like seeing them as full coordinates, and leaving the fact these happen to be another project up to gradle substitutions.

Tim-Brooks · 2018-08-10T17:07:54Z

Alright. I made more changes. core is now compile dependency of nio. I added back the exclude in x-pack core. And it looks like the netty exclusions are not necessary. I left the TODO comment.

rjernst

Thanks @tbrooks8, LGTM

rjernst · 2018-08-10T17:13:37Z

x-pack/plugin/core/build.gradle

    shadow 'com.unboundid:unboundid-ldapsdk:3.2.0'
    shadow project(path: ':modules:transport-netty4', configuration: 'runtime')
+    shadow(project(path: ':plugins:transport-nio', configuration: 'runtime')) {
+        // TODO: This should not be necessary if core is compile only


I would expand this comment a bit. Something like:

// TODO: core exclusion should not be necessary, since it is a transitive dep of all plugins

Also, I wonder if this is due to how we add server as a compile dep, but this is shadow here? I wonder if @nik9000's upcoming changes to add bundle will make this better?

…listeners * elastic/master: (58 commits) [ML] Partition-wise maximum scores (elastic#32748) [DOCS] XContentBuilder#bytes method removed, using BytesReference.bytes(docBuilder) (elastic#32771) HLRC: migration get assistance API (elastic#32744) Add a task to run forbiddenapis using cli (elastic#32076) [Kerberos] Add debug log statement for exceptions (elastic#32663) Make x-pack core pull transport-nio (elastic#32757) Painless: Clean Up Whitelist Names (elastic#32791) Cat apis: Fix index creation time to use strict date format (elastic#32510) Clear Job#finished_time when it is opened (elastic#32605) (elastic#32755) Test: Only sniff host metadata for node_selectors (elastic#32750) Update scripted metric docs to use `state` variable (elastic#32695) Painless: Clean up PainlessCast (elastic#32754) [TEST] Certificate NONE not allowed in FIPS JVM (elastic#32753) [ML] Refactor ProcessCtrl into Autodetect and Normalizer builders (elastic#32720) Access build tools resources (elastic#32201) Tests: Disable rolling upgrade tests with system key on fips JVM (elastic#32775) HLRC: Ban LoggingDeprecationHandler (elastic#32756) Fix test reproducability in AbstractBuilderTestCase setup (elastic#32403) Only require java<version>_home env var if needed Tests: Muted ScriptDocValuesDatesTests.testJodaTimeBwc ...

Make x-pack core pull transport-nio

825537f

The security nio transports depend on transport-nio. This commit modifies x-pack core to include the transport-nio jar into the x-pack core module.

Tim-Brooks added >non-issue review v7.0.0 :Security/TLS SSL/TLS, Certificates labels Aug 9, 2018

Tim-Brooks requested review from rjernst and s1monw August 9, 2018 19:53

rjernst approved these changes Aug 9, 2018

View reviewed changes

Tim-Brooks added 2 commits August 9, 2018 15:57

Add comment

b9b3f60

Merge remote-tracking branch 'upstream/master' into fix_x_pack_with_nio

d2f54c6

alpar-t reviewed Aug 10, 2018

View reviewed changes

danielmitterdorfer mentioned this pull request Aug 10, 2018

Reduce garbage for requests with unpooled buffers #32228

Closed

Tim-Brooks added 2 commits August 10, 2018 10:11

Make changes

dc62e9d

Whitespace

b2d0b53

Tim-Brooks requested review from alpar-t and rjernst August 10, 2018 16:13

Changes

94b29b4

rjernst approved these changes Aug 10, 2018

View reviewed changes

Take ryans comment

803b11c

Tim-Brooks merged commit 38ec0ff into elastic:master Aug 10, 2018

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Tim-Brooks deleted the fix_x_pack_with_nio branch December 18, 2019 14:44

Conversation

Tim-Brooks commented Aug 9, 2018

Uh oh!

elasticmachine commented Aug 9, 2018

Uh oh!

Tim-Brooks commented Aug 9, 2018

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

rjernst Aug 9, 2018

Choose a reason for hiding this comment

Uh oh!

alpar-t Aug 10, 2018

Choose a reason for hiding this comment

Uh oh!

rjernst Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpar-t commented Aug 10, 2018

Uh oh!

Tim-Brooks commented Aug 10, 2018

Uh oh!

rjernst commented Aug 10, 2018

Uh oh!

rjernst commented Aug 10, 2018

Uh oh!

Tim-Brooks commented Aug 10, 2018

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

rjernst Aug 10, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rjernst Aug 10, 2018 •

edited

Loading