Add elasticsearch-nio jar for base nio classes by Tim-Brooks · Pull Request #27801 · elastic/elasticsearch

Tim-Brooks · 2017-12-14T00:10:30Z

This is related to #27802. This commit adds a jar called
elasticsearch-nio that contains the base nio classes that will be used
for the tcp nio transport and eventually the http nio transport.

The jar does not depend on elasticsearch:core, so all references to core
have been removed.

Tim-Brooks · 2017-12-14T00:16:30Z

A couple notes:

Using /lib/elasticsearch-nio was just a starting point (suggested by @rjernst). It can definitely be moved around if there is a better place.
I replaced ActionListener with BiConsumer as we use CompletableFuture which takes BiConsumer. Unfortunately CompletableFuture allows Throwable, so maybe I should introduce an NioListener that is similar to ActionListener?
Right now we rely on mocksocket for compile because putting socket calls directly in the elasticsearch-nio code breaks intellij test runner (we provide socketpermissions to jars when we set up policies, but intellij just uses the source set when running tests). I'm not sure what the long term solution to this is other than breaking those tests in intellij or making our policy installer support non-jar source sets.
Do we only want this for 7.0 or also 6.x? Keeping it on 7.0 complicates backports. I know we do not want a nio-transport plugin on 6.x, but I did not know about this base jar?

s1monw · 2017-12-15T09:46:07Z

I replaced ActionListener with BiConsumer as we use CompletableFuture which takes BiConsumer. Unfortunately CompletableFuture allows Throwable, so maybe I should introduce an NioListener that is similar to ActionListener?

We can do this but I feel like in a followup we should have a /lib/elasticsearch-common where we put all our utils in? WE can for now just copy ActionListener

Right now we rely on mocksocket for compile because putting socket calls directly in the elasticsearch-nio code breaks intellij test runner (we provide socketpermissions to jars when we set up policies, but intellij just uses the source set when running tests). I'm not sure what the long term solution to this is other than breaking those tests in intellij or making our policy installer support non-jar source sets.

I think that's ok for now.

Do we only want this for 7.0 or also 6.x? Keeping it on 7.0 complicates backports. I know we do not want a nio-transport plugin on 6.x, but I did not know about this base jar?

I'd leave 6.x alone and just not backport stuff anymore there unless its simple.

s1monw · 2017-12-15T09:47:32Z

libs/elasticsearch-nio/src/main/java/org/elasticsearch/nio/InboundChannelBuffer.java

                }
            }
-            ExceptionsHelper.rethrowAndSuppress(closingExceptions);


Honestly I think we should just copy these helper classes 1 to 1 and then work on a common-lib jar. It's time for it anyway @rjernst @jasontedor WDYT?

@s1monw I think it's probably time too, but I would want to use this opportunity to mark as much as possible in an internal package so that we are free to break whenever we want (obviously ActionListener would not be in such a package). When we go JDK 9 minimum, we can take this further.

Can we separate the steps?

@s1monw All I am suggesting is that when we move something (e.g., a utility class) that does not need to be part of the public API (because it's for example not used in the client or the plugin API) that we mark it internal. Nothing more. 😄

Tim-Brooks · 2017-12-18T21:12:19Z

We can do this but I feel like in a followup we should have a /lib/elasticsearch-common where we put all our utils in? WE can for now just copy ActionListener

@s1monw This is actually not perfectly straightforward. If I introduce an ActionListener in the nio jar, I have to also introduce a common.ActionListener -> nio.ActionListener conversion (and then a nio.ActionListener -> BiConsumer conversion). Maybe I should just leave it with BiConsumer for now, and start using ActionListener once that common jar is merged?

s1monw · 2017-12-19T15:17:44Z

@s1monw This is actually not perfectly straightforward. If I introduce an ActionListener in the nio jar, I have to also introduce a common.ActionListener -> nio.ActionListener conversion (and then a nio.ActionListener -> BiConsumer conversion). Maybe I should just leave it with BiConsumer for now, and start using ActionListener once that common jar is merged?

@tbrooks8 I am ok with that

rjernst

I have a few comments on the gradle side.

rjernst · 2017-12-20T17:09:17Z

libs/elasticsearch-nio/build.gradle

+apply plugin: 'nebula.maven-base-publish'
+apply plugin: 'nebula.maven-scm'
+
+targetCompatibility = JavaVersion.VERSION_1_8


This shouldn't be necessary, it is the default set by elasticsearch.build

rjernst · 2017-12-20T17:09:28Z

libs/elasticsearch-nio/build.gradle

+targetCompatibility = JavaVersion.VERSION_1_8
+sourceCompatibility = JavaVersion.VERSION_1_8
+
+group = 'org.elasticsearch'


Same here, not necessary.

rjernst · 2017-12-20T17:10:35Z

libs/elasticsearch-nio/build.gradle

+}
+
+// TODO: Currently disable licenses check as we rely on mocksocket
+dependencyLicenses.enabled = false


What does the license check being disabled have to do with mocksocket?

Mocksocket would need the license, sha, and notice. The mocksocket repository does not currently have a “notice” which is why I disabled the license check for right now.

I can add a notice, but I imagine we don’t want to actually release this depending on mocksocket.

rjernst · 2017-12-20T17:11:39Z

libs/elasticsearch-nio/build.gradle

+
+dependencies {
+  compile "org.apache.logging.log4j:log4j-api:${versions.log4j}"
+  compile "org.elasticsearch:mocksocket:${versions.mocksocket}"


This means mocksocket would need to be published and a production dependency of the transport-nio module right? That seems backwards to me. Why wouldn't the tie in to mocksocket be inside test framework?

This is explained in this comment. Our permission code applies permissions to jars. I can give all of elasticsearch-nio socket permissions, but the IntelliJ test runner does not compile that to a jar. So we do not properly apply permissions and tests are broken in IntelliJ.

I don’t think we want to release with this depending on mocksocket. I just did not see an immediate resolution for that issue in this PR.

rjernst

LGTM

rjernst · 2017-12-20T20:56:06Z

libs/elasticsearch-nio/build.gradle

+apply plugin: 'nebula.maven-base-publish'
+apply plugin: 'nebula.maven-scm'
+
+group = 'org.elasticsearch'


This shouldn't be necessary.

jasontedor

LGTM.

jasontedor · 2017-12-20T21:04:59Z

libs/elasticsearch-nio/build.gradle

+}
+
+//JarHell is part of es core, which we don't want to pull in
+jarHell.enabled=false


I will open a separate issue for this.

I opened #27933.

jasontedor · 2017-12-20T21:06:27Z

libs/elasticsearch-nio/src/main/java/org/elasticsearch/nio/utils/ExceptionsHelper.java

+
+import java.util.List;
+
+public class ExceptionsHelper {


Maybe add TODOs for these to be in the core JAR?

* es/master: (45 commits) Adapt scroll rest test after backport. relates #27842 Move early termination based on index sort to TopDocs collector (#27666) Upgrade beats templates that we use for bwc testing. (#27929) ingest: upgraded ingest geoip's geoip2's dependencies. [TEST] logging for update by query test #27820 Add elasticsearch-nio jar for base nio classes (#27801) Use full profile on JDK 10 builds Require Gradle 4.3 Enable grok processor to support long, double and boolean (#27896) Add unreleased v6.1.2 version TEST: reduce blob size #testExecuteMultipartUpload Check index under the store metadata lock (#27768) Fixes DocStats to not report index size < -1 (#27863) Fixed test to be up to date with the new database files. Upgrade to Lucene 7.2.0. (#27910) Disable TestZenDiscovery in cloud providers integrations test Use `_refresh` to shrink the version map on inactivity (#27918) Make KeyedLock reentrant (#27920) ingest: Upgraded the geolite2 databases. [Test] Fix IndicesClientDocumentationIT (#27899) ...

* Splits nio project into two for eclipse build only #27801 introduced a new gradle project `:libs:elasticsearch-nio` which creates cyclical project dependencies when importingthe projects into Eclipse. This change applies the same trick as we have for the core project where, and building for Eclipse, splits the `:libs:elasticsearch-nio` project into `:libs:elasticsearch-nio` which points to `src/main` and `:libs:elasticsearch-nio-tests` which points to `src/test`. This prevents cyclical project dependencies in Eclipse arising from the fact that eclipse does not separate compile/runtime dependencies from test dependencies. * Removes integTest bits since there are none

Tim-Brooks added 4 commits December 13, 2017 11:52

WIP

88444a6

WIP

a82314a

WIP

31cdc9e

A few cleanups

1f9d794

Tim-Brooks added :Distributed/Network Http and internode communication implementations >enhancement review v7.0.0 labels Dec 14, 2017

Tim-Brooks requested review from jasontedor, rjernst and s1monw December 14, 2017 00:10

Tim-Brooks added 2 commits December 13, 2017 17:17

Don't currently need socket permissions for elasticsearch-nio

075d81c

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

00e19ba

s1monw reviewed Dec 15, 2017

View reviewed changes

Tim-Brooks added 3 commits December 15, 2017 11:16

Merge nio group

b48f467

Cleanups

0afab48

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

7a30220

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

dcce8fb

Remove comment

71ddafa

Tim-Brooks requested a review from s1monw December 20, 2017 15:04

s1monw approved these changes Dec 20, 2017

View reviewed changes

rjernst reviewed Dec 20, 2017

View reviewed changes

Tim-Brooks added 2 commits December 20, 2017 14:24

Changes based on review

58f6e40

Fix permissions

adda661

rjernst approved these changes Dec 20, 2017

View reviewed changes

Drop group assignment

4a4ee20

jasontedor approved these changes Dec 20, 2017

View reviewed changes

Add todo comment

5d1800a

Tim-Brooks merged commit 06b3130 into elastic:master Dec 20, 2017

colings86 mentioned this pull request Dec 21, 2017

Splits nio project into two for eclipse build only #27939

Merged

ywelsch mentioned this pull request Dec 28, 2017

Running integration tests with elasticsearch-nio fails in Intellij #28011

Closed

Tim-Brooks deleted the work_on_base_nio branch December 10, 2018 16:19

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

Conversation

Tim-Brooks commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tim-Brooks commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s1monw commented Dec 15, 2017

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!

Tim-Brooks commented Dec 18, 2017

Uh oh!

s1monw commented Dec 19, 2017

Uh oh!

rjernst 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!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Tim-Brooks commented Dec 14, 2017 •

edited

Loading

Tim-Brooks commented Dec 14, 2017 •

edited

Loading