Fixed compression support for h2c protocol #4944

reta · 2022-10-26T19:33:55Z

Signed-off-by: Andriy Redko [email protected]

Description

Fixed compression support for h2c protocol (upgrade handler). There were 2 issues (quite nasty ones, to be fair):

the decoder_compress handler was run after aggregator and was not able to decode the request
the encoder is not needed, it is already present in the HttpServerCodec configured before

Why our tests did not catch it: the default CloseableHttpAsyncClient does not support compression out of the box so that applies to RestClient and RestHighLevelClient which are used for all kind of tests.

To check the compression works on both sides, crafting the request using CloseableHttpClient instead which uses compression by default (or 3rd party clients like https://github.com/opensearch-project/opensearch-go).

Issues Resolved

Closes opensearch-project/opensearch-go#163

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

reta · 2022-10-26T19:35:07Z

@VachaShah fyi

github-actions · 2022-10-26T19:39:40Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5707/
CommitID: 0ba4dbc
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-10-26T19:39:42Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5708/
CommitID: 845d6ea
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-10-26T20:04:02Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5709/
CommitID: ef75775
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-10-26T20:33:17Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5712/
CommitID: 2d17116
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

andrross · 2022-10-26T21:19:17Z

...les/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java

@@ -413,10 +413,9 @@ protected void channelRead0(ChannelHandlerContext ctx, HttpMessage msg) throws E
                    // If this handler is hit then no upgrade has been attempted and the client is just talking HTTP
                    final ChannelPipeline pipeline = ctx.pipeline();
                    pipeline.addAfter(ctx.name(), "handler", getRequestHandler());
-                    pipeline.replace(this, "aggregator", aggregator);
+                    pipeline.replace(this, "decoder_compress", new HttpContentDecompressor());


I haven't looked through this deeply yet so apologies in advance if this comment is not helpful...but I find this all this "replace", "addAfter", "addBefore", etc makes it very hard to reason about the state of the pipeline when this is done. I'm wondering if it is possible to simplify this.

One thing that jumps out is that we're adding handlers before "handler" (which is added above). Could that be re-written as the following where it's more of a simple chaining of handlers?

final ChannelPipeline pipeline = ctx.pipeline(); pipeline.addAfter(ctx.name(), "request_creator", ... pipeline.addAfter("request_creator", "response_creator", ... pipeline.addAfter("response_creator", "pipelining", ... pipeline.addAfter("pipelining", "handler", ...

Also sometimes we're using the pipeline local variable and sometimes we're using ch.pipeline(). I'm assuming those refer to the same instance?

In any case, making the Netty pipeline setup as simple as possible can really help with long term maintainability.

Even better, if we know that ctx.name() is the last handler in the pipeline (this handler was just added via an addLast call so I think it should be), then you can just add the new handlers via addLast calls, which I find is the simplest way to construct a pipeline.

Thanks @andrross

Also sometimes we're using the pipeline local variable and sometimes we're using ch.pipeline(). I'm assuming those refer to the same instance?

Yes, thanks for noticing, I will change that to use pipeline in this block

One thing that jumps out is that we're adding handlers before "handler" (which is added above). Could that be re-written as the following where it's more of a simple chaining of handlers?

The order of handler is super important, and in this particular block - we really need to replace the handler and restart the processing again. The whole kind of purpose of that is to process the upgrade request first and than run the original request as it have been processed the normal way.

Even better, if we know that ctx.name() is the last handler in the pipeline (this handler was just added via an addLast call so I think it should be), then you can just add the new handlers via addLast calls, which I find is the simplest way to construct a pipeline.

The ctx.name() is the current handler which we are replacing with decoder_compress, there are a few other handlers added before and/or after (by means of HttpServerUpgradeHandler and CleartextHttp2ServerUpgradeHandler).

The order of handler is super important

Understood. I'm trying to see if there's a more straightforward way to achieve the desired order so that the actual order is more apparent/easier to understand when reading the code. It looks to me light we add "handler" to the pipeline, then 10 lines later start adding handlers before it. Seems like that could all happen together, and potentially invert the order of adding them so that you don't have to mix "addAfter" and "addBefore".

I think in general it is possible to rewrite the pipeline configuration a bit, it took me a while to understand why some handlers are either not invoked or invoked when they shouldn't, luckily we have test for it now :-) (not like affraid to touch it but it took me really a lot of time today to troubleshoot) :-)

Fair enough...I did start this comment with "apologies in advance" :)

Fair enough...I did start this comment with "apologies in advance" :)

Thanks a lot for review

andrross · 2022-10-26T23:28:26Z

CHANGELOG.md

@@ -172,6 +172,9 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - Fix a bug on handling an invalid array value for point type field #4900([#4900](https://github.com/opensearch-project/OpenSearch/pull/4900))
 - [BUG]: Allow decommission to support delay timeout ([#4930](https://github.com/opensearch-project/OpenSearch/pull/4930))
 - Fix failing test: VerifyVersionConstantsIT ([#4946](https://github.com/opensearch-project/OpenSearch/pull/4946))
+- [BUG]: Allow decommission to support delay timeout [#4930](https://github.com/opensearch-project/OpenSearch/pull/4930))


Extra line?

Sorry, "extra line" referred to this "Allow decommission..." line which shouldn't come in as a part of this PR

Oh right, sorry about that, removed :)

andrross · 2022-10-26T23:28:44Z

...les/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java

@@ -413,10 +413,9 @@ protected void channelRead0(ChannelHandlerContext ctx, HttpMessage msg) throws E
                    // If this handler is hit then no upgrade has been attempted and the client is just talking HTTP
                    final ChannelPipeline pipeline = ctx.pipeline();
                    pipeline.addAfter(ctx.name(), "handler", getRequestHandler());
-                    pipeline.replace(this, "aggregator", aggregator);
+                    pipeline.replace(this, "decoder_compress", new HttpContentDecompressor());


Fair enough...I did start this comment with "apologies in advance" :)

reta · 2022-10-26T23:30:03Z

@andrross thanks a lot for review

github-actions · 2022-10-26T23:46:13Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/5728/
CommitID: f52958e

codecov-commenter · 2022-10-26T23:49:48Z

Codecov Report

Merging #4944 (99b72ad) into main (782dc59) will decrease coverage by 0.53%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #4944      +/-   ##
============================================
- Coverage     71.37%   70.83%   -0.54%     
+ Complexity    58338    57917     -421     
============================================
  Files          4689     4689              
  Lines        277022   277000      -22     
  Branches      40315    40311       -4     
============================================
- Hits         197718   196213    -1505     
- Misses        63304    64547    +1243     
- Partials      16000    16240     +240

Impacted Files	Coverage Δ
...nsearch/http/netty4/Netty4HttpServerTransport.java	`54.78% <100.00%> (+1.45%)`	⬆️
...g/opensearch/index/analysis/CharFilterFactory.java	`0.00% <0.00%> (-100.00%)`	⬇️
...adonly/AddIndexBlockClusterStateUpdateRequest.java	`0.00% <0.00%> (-75.00%)`	⬇️
...readonly/TransportVerifyShardIndexBlockAction.java	`9.75% <0.00%> (-58.54%)`	⬇️
.../org/opensearch/client/indices/AnalyzeRequest.java	`31.00% <0.00%> (-48.00%)`	⬇️
.../search/aggregations/pipeline/HoltLinearModel.java	`20.33% <0.00%> (-42.38%)`	⬇️
...h/action/ingest/SimulateDocumentVerboseResult.java	`60.71% <0.00%> (-39.29%)`	⬇️
...indices/readonly/TransportAddIndexBlockAction.java	`20.68% <0.00%> (-37.94%)`	⬇️
...cluster/coordination/PublishClusterStateStats.java	`33.33% <0.00%> (-37.51%)`	⬇️
.../opensearch/client/indices/CloseIndexResponse.java	`56.25% <0.00%> (-35.00%)`	⬇️
... and 452 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

github-actions · 2022-10-27T00:03:58Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5731/
CommitID: 7386a3f
Please examine the workflow log, locate, and copy-paste the failure below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-10-27T01:01:49Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/5744/
CommitID: 7386a3f

Signed-off-by: Andriy Redko <[email protected]>

github-actions · 2022-10-27T13:01:16Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/5786/
CommitID: 99b72ad

reta added bug Something isn't working v3.0.0 Issues and PRs related to version 3.0.0 labels Oct 26, 2022

reta requested a review from a team as a code owner October 26, 2022 19:33

reta force-pushed the issue-163 branch from 0ba4dbc to 845d6ea Compare October 26, 2022 19:35

reta force-pushed the issue-163 branch from 845d6ea to ef75775 Compare October 26, 2022 19:35

reta force-pushed the issue-163 branch from ef75775 to 2d17116 Compare October 26, 2022 20:02

Poojita-Raj mentioned this pull request Oct 26, 2022

[BUG] Failing test on gradle check: org.opensearch.qa.verify_version_constants.VerifyVersionConstantsIT.testLuceneVersionConstant #4945

Closed

andrross reviewed Oct 26, 2022

View reviewed changes

reta force-pushed the issue-163 branch from 2d17116 to f52958e Compare October 26, 2022 23:15

andrross reviewed Oct 26, 2022

View reviewed changes

reta added 2 commits October 27, 2022 08:23

Fixed compression support for h2c protocol

7151af0

Signed-off-by: Andriy Redko <[email protected]>

Removed extra empty line in CHANGELOG

99b72ad

Signed-off-by: Andriy Redko <[email protected]>

reta force-pushed the issue-163 branch from 7386a3f to 99b72ad Compare October 27, 2022 12:24

andrross approved these changes Oct 27, 2022

View reviewed changes

andrross merged commit 84aba05 into opensearch-project:main Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed compression support for h2c protocol #4944

Fixed compression support for h2c protocol #4944

reta commented Oct 26, 2022 •

edited

Loading

reta commented Oct 26, 2022

github-actions bot commented Oct 26, 2022

github-actions bot commented Oct 26, 2022

github-actions bot commented Oct 26, 2022

github-actions bot commented Oct 26, 2022

andrross Oct 26, 2022 •

edited

Loading

andrross Oct 26, 2022

reta Oct 26, 2022

andrross Oct 26, 2022

reta Oct 26, 2022

andrross Oct 26, 2022

reta Oct 26, 2022

andrross Oct 26, 2022

reta Oct 26, 2022

andrross Oct 27, 2022

reta Oct 27, 2022

andrross Oct 26, 2022

reta commented Oct 26, 2022

github-actions bot commented Oct 26, 2022

codecov-commenter commented Oct 26, 2022 •

edited

Loading

github-actions bot commented Oct 27, 2022

github-actions bot commented Oct 27, 2022

github-actions bot commented Oct 27, 2022

Fixed compression support for h2c protocol #4944

Fixed compression support for h2c protocol #4944

Conversation

reta commented Oct 26, 2022 • edited Loading

Description

Issues Resolved

Check List

reta commented Oct 26, 2022

github-actions bot commented Oct 26, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 26, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 26, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 26, 2022

Gradle Check (Jenkins) Run Completed with:

andrross Oct 26, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta commented Oct 26, 2022

github-actions bot commented Oct 26, 2022

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Oct 26, 2022 • edited Loading

Codecov Report

github-actions bot commented Oct 27, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 27, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 27, 2022

Gradle Check (Jenkins) Run Completed with:

reta commented Oct 26, 2022 •

edited

Loading

andrross Oct 26, 2022 •

edited

Loading

codecov-commenter commented Oct 26, 2022 •

edited

Loading