Fix HTTP/2 Upgrade flow for requests with content (post/put/...) #7105

reta · 2023-04-11T18:47:14Z

Description

[Describe what this change achieves]

Issues Resolved

[List any issues this PR will resolve]

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 · 2023-04-11T18:50:08Z

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

+            final HttpServerUpgradeHandler upgradeHandler = new HttpServerUpgradeHandler(
+                sourceCodec,
+                upgradeCodecFactory,
+                handlingSettings.getMaxContentLength()


Found this one while prototyping #3000: by default, Netty assumes the upgrade is initiated by GET requests, but in case of OpenSearch, that is not necessarily the case.

github-actions · 2023-04-11T18:50:56Z

Gradle Check (Jenkins) Run Completed with:

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

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

andrross · 2023-04-11T18:54:39Z

...nsport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http2PostIT.java

+        return false; // enable http
+    }
+
+    public void testThatNettyHttpServerSupportsHttp2() throws Exception {


Why not make the post operation another test case in Netty4Http2IT as opposed to creating a separate test class?

Because we need "clean state" when no upgrade happened yet, if we bundle both tests cases, the server side upgrade flow would be already exercised by one of the test cases.

Scope.TEST doesn't give you that clean state on each test method?

Hm ... that's interesting, it should but I have test run issues ... let me check

Should be good now, the failure was caused by client state (https://github.com/opensearch-project/OpenSearch/pull/7105/files#diff-2916f5f80f95ddf8143e294a081d207c058c16e3e2dd637cf6502feb60bf36b2R373), not server state, fixed now

andrross · 2023-04-11T19:03:52Z

...nsport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http2PostIT.java

+        }
+    }
+
+    private void assertOpaqueIdsInAnyOrder(Collection<String> opaqueIds) {


I think you could replace this whole method with assertThat(opaqueIds, equalTo(List.of("0")));, no? The matcher should give a good error message by default.

or even assertThat(opaqueIds, contains("0"));

Sure, thanks!

github-actions · 2023-04-11T19:26:27Z

Gradle Check (Jenkins) Run Completed with:

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

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

github-actions · 2023-04-11T19:56:44Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.repositories.azure.AzureBlobContainerRetriesTests.testReadBlobWithRetries

URL: https://build.ci.opensearch.org/job/gradle-check/13880/
CommitID: 7d696cd
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

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

codecov-commenter · 2023-04-11T20:02:08Z

Codecov Report

Merging #7105 (af69e95) into main (3ba333b) will decrease coverage by 0.18%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #7105      +/-   ##
============================================
- Coverage     70.77%   70.60%   -0.18%     
+ Complexity    59367    59226     -141     
============================================
  Files          4825     4825              
  Lines        284369   284370       +1     
  Branches      41021    41021              
============================================
- Hits         201263   200780     -483     
- Misses        66603    67014     +411     
- Partials      16503    16576      +73

Impacted Files	Coverage Δ
...nsearch/http/netty4/Netty4HttpServerTransport.java	`56.08% <100.00%> (+0.23%)`	⬆️

... and 500 files with indirect coverage changes

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

andrross · 2023-04-11T20:13:31Z

.../transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http2IT.java

+        }
+    }
+
+    private void assertOpaqueIdsInAnyOrder(int expected, Collection<String> opaqueIds) {
        // check if opaque ids are present in any order, since for HTTP/2 we use streaming (no head of line blocking)
        // and responses may come back at any order
        int i = 0;


I think this local variable is unused

andrross · 2023-04-11T20:15:25Z

.../transport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http2IT.java

+        }
+    }
+
+    private void assertOpaqueIdsInAnyOrder(int expected, Collection<String> opaqueIds) {
        // check if opaque ids are present in any order, since for HTTP/2 we use streaming (no head of line blocking)
        // and responses may come back at any order
        int i = 0;
        String msg = String.format(Locale.ROOT, "Expected list of opaque ids to be in any order, got [%s]", opaqueIds);


Not a big deal, but I think you can remove this because assertThat will generate a good error message by default

andrross · 2023-04-11T20:16:12Z

...nsport-netty4/src/internalClusterTest/java/org/opensearch/http/netty4/Netty4Http2PostIT.java

+import static org.hamcrest.Matchers.hasSize;
+
+@ClusterScope(scope = Scope.TEST, supportsDedicatedMasters = false, numDataNodes = 1)
+public class Netty4Http2PostIT extends OpenSearchNetty4IntegTestCase {


Can this be removed now?

Oh sure, git played with me here :(

github-actions · 2023-04-11T20:29:14Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/13882/
CommitID: 1bd7d82

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

github-actions · 2023-04-11T21:02:07Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.index.IndexServiceTests.testAsyncTranslogTrimTaskOnClosedIndex

URL: https://build.ci.opensearch.org/job/gradle-check/13889/
CommitID: af69e95
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

…nsearch-project#7105) * Fix HTTP/2 Upgrade flow for requests with content (post/put/...) Signed-off-by: Andriy Redko <[email protected]> * Address code review comments Signed-off-by: Andriy Redko <[email protected]> * Address code review comments Signed-off-by: Andriy Redko <[email protected]> * Address code review comments Signed-off-by: Andriy Redko <[email protected]> --------- Signed-off-by: Andriy Redko <[email protected]>

reta added bug Something isn't working skip-changelog labels Apr 11, 2023

reta requested review from anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, gbbafna, setiah, kartg, kotwanikunal, mch2, nknize, owaiskazi19, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja, dreamer-89, tlfeng, VachaShah and xuezhou25 as code owners April 11, 2023 18:47

reta commented Apr 11, 2023

View reviewed changes

Fix HTTP/2 Upgrade flow for requests with content (post/put/...)

b23035a

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

reta force-pushed the upgrade.post branch from 1d95873 to b23035a Compare April 11, 2023 18:52

andrross reviewed Apr 11, 2023

View reviewed changes

Address code review comments

7d696cd

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

Address code review comments

1bd7d82

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

andrross reviewed Apr 11, 2023

View reviewed changes

Address code review comments

af69e95

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

andrross approved these changes Apr 11, 2023

View reviewed changes

reta merged commit a15475a into opensearch-project:main Apr 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HTTP/2 Upgrade flow for requests with content (post/put/...) #7105

Fix HTTP/2 Upgrade flow for requests with content (post/put/...) #7105

reta commented Apr 11, 2023

reta Apr 11, 2023

github-actions bot commented Apr 11, 2023

andrross Apr 11, 2023

reta Apr 11, 2023 •

edited

Loading

andrross Apr 11, 2023

reta Apr 11, 2023

reta Apr 11, 2023

andrross Apr 11, 2023

andrross Apr 11, 2023

reta Apr 11, 2023

github-actions bot commented Apr 11, 2023

github-actions bot commented Apr 11, 2023

codecov-commenter commented Apr 11, 2023 •

edited

Loading

andrross Apr 11, 2023

andrross Apr 11, 2023

andrross Apr 11, 2023

reta Apr 11, 2023

github-actions bot commented Apr 11, 2023

github-actions bot commented Apr 11, 2023

Fix HTTP/2 Upgrade flow for requests with content (post/put/...) #7105

Fix HTTP/2 Upgrade flow for requests with content (post/put/...) #7105

Conversation

reta commented Apr 11, 2023

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

github-actions bot commented Apr 11, 2023

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

reta Apr 11, 2023 • 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

github-actions bot commented Apr 11, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Apr 11, 2023

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Apr 11, 2023 • edited Loading

Codecov Report

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

github-actions bot commented Apr 11, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Apr 11, 2023

Gradle Check (Jenkins) Run Completed with:

reta Apr 11, 2023 •

edited

Loading

codecov-commenter commented Apr 11, 2023 •

edited

Loading