Make http pipelining support mandatory #30695

Tim-Brooks · 2018-05-17T17:15:17Z

This is related to #29500 and #28898. This commit removes the abilitiy
to disable http pipelining. After this commit, any elasticsearch node
will support pipelined requests from a client. Additionally, it extracts
some of the http pipelining work to the server module. This extracted
work is used to implement pipelining for the nio plugin.

elasticmachine · 2018-05-17T17:15:23Z

Pinging @elastic/es-core-infra

…_nio

s1monw

I think this looks good. I left some nit-picks and I think it's good to have a single codepath here. I think for the removed setting you need to add something to the migration docs and since @spinscale implemented this in the first place he should take a look too. thanks for cleaning this up

s1monw · 2018-05-18T08:10:25Z

...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java

+                ctx.write(readyResponse.v1().getResponse(), readyResponse.v2());
+            }
+        } catch (IllegalStateException e) {
+            ctx.channel().close();


hmm why do you close this twice? and if so should we use try/finally?

Did not mean to close twice. Removing the second throw.

s1monw · 2018-05-18T08:10:55Z

...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java

+            List<Tuple<Netty4HttpResponse, ChannelPromise>> readyResponses = aggregator.write(response, promise);
+            success = true;
+            for (Tuple<Netty4HttpResponse, ChannelPromise> readyResponse : readyResponses) {
+                ctx.write(readyResponse.v1().getResponse(), readyResponse.v2());


can this also throw an IOException?

No. It does not throw a checked exception.

It passes it on to the next write handler in the netty pipeline.

spinscale · 2018-05-18T08:31:05Z

...ns/transport-nio/src/test/java/org/elasticsearch/http/nio/NioHttpPipeliningHandlerTests.java

+    public void tearDown() throws Exception {
+        waitingRequests.keySet().forEach(this::finishRequest);
+        shutdownExecutorService();
+        super.tearDown();


if you rename this method to something else, it will be run first after the test and the super.tearDown will be run after that without the need to specify it (unless I missed something obvious here).

spinscale · 2018-05-18T09:26:36Z

Did a pass over it and this looks so much nicer than before!

…_nio

* master: QA: Add xpack tests to rolling upgrade (#30795) Modify state of VerifyRepositoryResponse for bwc (#30762) Reduce CLI scripts to one-liners on Windows (#30772) Simplify number of shards setting (#30783) Replace Request#setHeaders with addHeader (#30588) [TEST] remove endless wait in RestClientTests (#30776) [Docs] Fix script-fields snippet execution (#30693) Upgrade to Lucene-7.4.0-snapshot-cc2ee23050 (#30778) [DOCS] Add SAML configuration information (#30548) [DOCS] Remove X-Pack references from SQL CLI (#30694) Make http pipelining support mandatory (#30695) [Docs] Fix typo in circuit breaker docs (#29659) [Feature] Adding a char_group tokenizer (#24186) [Docs] Fix broken cross link in documentation Test: wait for netty threads in a JUnit ClassRule (#30763) Increase the maximum number of filters that may be in the cache. (#30655) [Security] Include an empty json object in an json array when FLS filters out all fields (#30709) [TEST] Wait for CS to be fully applied in testDeleteCreateInOneBulk Add more yaml tests for get alias API (#29513) Ignore empty completion input (#30713) [DOCS] fixed incorrect default [ML] Filter undefined job groups from update calendar actions (#30757) Fix docs failure on language analyzers (#30722) [Docs] Fix inconsistencies in snapshot/restore doc (#30480) Enable installing plugins from snapshots.elastic.co (#30765) Remove fedora 26, add 28 (#30683) Accept Gradle build scan agreement (#30645) Remove logging from elasticsearch-nio jar (#30761) Add Delete Repository High Level REST API (#30666)

This reverts commit 31251c9 introduced in #30695. We suspect this commit is causing the OOME's reported in #30811 and we will use this PR to test this assertion.

…" (elastic#30813)" This reverts commit 4fd0a3e.

* master: [DOCS] Splits auditing.asciidoc into smaller files Reintroduce mandatory http pipelining support (elastic#30820) Painless: Types Section Clean Up (elastic#30283) Add support for indexed shape routing in geo_shape query (elastic#30760) [test] java tests for archive packaging (elastic#30734) Revert "Make http pipelining support mandatory (elastic#30695)" (elastic#30813) [DOCS] Fix more edit URLs in Stack Overview (elastic#30704) Use correct cluster state version for node fault detection (elastic#30810) Change serialization version of doc-value fields. [DOCS] Fixes broken link for native realm [DOCS] Clarified audit.index.client.hosts (elastic#30797) [TEST] Don't expect acks when isolating nodes

* master: (25 commits) [DOCS] Splits auditing.asciidoc into smaller files Reintroduce mandatory http pipelining support (elastic#30820) Painless: Types Section Clean Up (elastic#30283) Add support for indexed shape routing in geo_shape query (elastic#30760) [test] java tests for archive packaging (elastic#30734) Revert "Make http pipelining support mandatory (elastic#30695)" (elastic#30813) [DOCS] Fix more edit URLs in Stack Overview (elastic#30704) Use correct cluster state version for node fault detection (elastic#30810) Change serialization version of doc-value fields. [DOCS] Fixes broken link for native realm [DOCS] Clarified audit.index.client.hosts (elastic#30797) [TEST] Don't expect acks when isolating nodes Add a `format` option to `docvalue_fields`. (elastic#29639) Fixes UpdateSettingsRequestStreamableTests mutate bug Mustes {p0=snapshot.get_repository/10_basic/*} YAML test Revert "Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled" Only allow x-pack metadata if all nodes are ready (elastic#30743) Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled Use original settings on full-cluster restart (elastic#30780) Only ack cluster state updates successfully applied on all nodes (elastic#30672) ...

* master: [DOCS] Fixes typos in security settings Fix GeoShapeQueryBuilder serialization after backport [DOCS] Splits auditing.asciidoc into smaller files Reintroduce mandatory http pipelining support (#30820) Painless: Types Section Clean Up (#30283) Add support for indexed shape routing in geo_shape query (#30760) [test] java tests for archive packaging (#30734) Revert "Make http pipelining support mandatory (#30695)" (#30813) [DOCS] Fix more edit URLs in Stack Overview (#30704) Use correct cluster state version for node fault detection (#30810) Change serialization version of doc-value fields. [DOCS] Fixes broken link for native realm [DOCS] Clarified audit.index.client.hosts (#30797) [TEST] Don't expect acks when isolating nodes Add a `format` option to `docvalue_fields`. (#29639) Fixes UpdateSettingsRequestStreamableTests mutate bug Mustes {p0=snapshot.get_repository/10_basic/*} YAML test Revert "Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled" Only allow x-pack metadata if all nodes are ready (#30743) Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled Use original settings on full-cluster restart (#30780) Only ack cluster state updates successfully applied on all nodes (#30672) Expose Lucene's FeatureField. (#30618) Fix a grammatical error in the 'search types' documentation. Remove http pipelining from integration test case (#30788)

* es/ccr: (55 commits) [DOCS] Fixes typos in security settings Fix GeoShapeQueryBuilder serialization after backport [DOCS] Splits auditing.asciidoc into smaller files Reintroduce mandatory http pipelining support (elastic#30820) Painless: Types Section Clean Up (elastic#30283) Add support for indexed shape routing in geo_shape query (elastic#30760) [test] java tests for archive packaging (elastic#30734) Revert "Make http pipelining support mandatory (elastic#30695)" (elastic#30813) [DOCS] Fix more edit URLs in Stack Overview (elastic#30704) Use correct cluster state version for node fault detection (elastic#30810) Change serialization version of doc-value fields. [DOCS] Fixes broken link for native realm [DOCS] Clarified audit.index.client.hosts (elastic#30797) [TEST] Don't expect acks when isolating nodes Mute CorruptedFileIT in CCR Add a `format` option to `docvalue_fields`. (elastic#29639) Fixes UpdateSettingsRequestStreamableTests mutate bug Mustes {p0=snapshot.get_repository/10_basic/*} YAML test Revert "Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled" Only allow x-pack metadata if all nodes are ready (elastic#30743) ...

Tim-Brooks added 7 commits May 16, 2018 14:57

Require pipelining

955e86f

Remove need for synchronized

56d196e

Work on nio pipelining

5a494bc

IMplement nio pipelining

09b86a3

Some reworks

30339f9

Remove references to setting

15eac78

tests

7adb2b3

Tim-Brooks added >enhancement review :Distributed/Network Http and internode communication implementations v7.0.0 labels May 17, 2018

Tim-Brooks requested review from s1monw, jaymode and jasontedor May 17, 2018 17:15

Tim-Brooks added 6 commits May 17, 2018 11:16

Rename handler

a884eaa

Add assertions and release request

7bad406

Merge remote-tracking branch 'upstream/master' into add_pipelining_to…

945ecb6

…_nio

Fix checkstyle

ba82266

Merge remote-tracking branch 'upstream/master' into add_pipelining_to…

91b7131

…_nio

Merge remote-tracking branch 'upstream/master' into add_pipelining_to…

3e0af93

…_nio

s1monw requested changes May 18, 2018

View reviewed changes

spinscale reviewed May 18, 2018

View reviewed changes

Changes for review

948841f

Tim-Brooks requested a review from s1monw May 18, 2018 16:30

Merge remote-tracking branch 'upstream/master' into add_pipelining_to…

86bafeb

…_nio

s1monw approved these changes May 22, 2018

View reviewed changes

Tim-Brooks merged commit 31251c9 into elastic:master May 22, 2018

This was referenced May 23, 2018

Revert "Make http pipelining support mandatory (#30695)" #30813

Merged

Docs test cluster going out of memory on reference/mapping/types/geo-shape docs #30811

Closed

Tim-Brooks added a commit to Tim-Brooks/elasticsearch that referenced this pull request May 23, 2018

Revert "Revert "Make http pipelining support mandatory (elastic#30695)…

f0833eb

…" (elastic#30813)" This reverts commit 4fd0a3e.

Tim-Brooks mentioned this pull request May 23, 2018

Reintroduce mandatory http pipelining support #30820

Merged

gwbrown mentioned this pull request Nov 29, 2018

Update Deprecation Info API for 7.0 breaking changes #36024

Closed

26 tasks

Tim-Brooks deleted the add_pipelining_to_nio branch December 10, 2018 16:20

gwbrown mentioned this pull request Dec 12, 2018

Deprecation check for HTTP pipelining #36521

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make http pipelining support mandatory #30695

Make http pipelining support mandatory #30695

Tim-Brooks commented May 17, 2018

elasticmachine commented May 17, 2018

s1monw left a comment

s1monw May 18, 2018

Tim-Brooks May 18, 2018

s1monw May 18, 2018

Tim-Brooks May 18, 2018

Tim-Brooks May 18, 2018

spinscale May 18, 2018

spinscale commented May 18, 2018

Make http pipelining support mandatory #30695

Make http pipelining support mandatory #30695

Conversation

Tim-Brooks commented May 17, 2018

elasticmachine commented May 17, 2018

s1monw left a comment

Choose a reason for hiding this comment

s1monw May 18, 2018

Choose a reason for hiding this comment

Tim-Brooks May 18, 2018

Choose a reason for hiding this comment

s1monw May 18, 2018

Choose a reason for hiding this comment

Tim-Brooks May 18, 2018

Choose a reason for hiding this comment

Tim-Brooks May 18, 2018

Choose a reason for hiding this comment

spinscale May 18, 2018

Choose a reason for hiding this comment

spinscale commented May 18, 2018