Cleanup HttpServerTransport.Dispatcher in Netty tests

[fdesu]

Description

This PR introduces helper builders TestDispatcherBuilder in modules/Netty & plugins/Reactor Netty that simplify the way instances of HttpTransport.Disaptcher get created.

It also includes some minor cleanups like remove stale throws clause from some of setup/teardown method and such.

Related Issues

This is a cleanup PR, no additional issue was created.

Check List

• Functionality includes testing.
• [ ] API changes companion pull request created, if applicable.
• [ ] Public documentation issue/PR created, if applicable.

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.

Summary by CodeRabbit

• Tests

  • Refactored HTTP transport tests to use a shared builder-style dispatcher utility, reducing duplication and standardizing test wiring across suites.

• Chores

  • Simplified test lifecycle method signatures and cleaned up imports across HTTP transport tests.
  • Added a changelog entry noting dispatcher cleanup in Netty tests.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds a TestDispatcherBuilder utility and replaces many anonymous HttpServerTransport.Dispatcher implementations in Netty4 and ReactorNetty4 transport tests with builder-based dispatchers; several test lifecycle methods (setup/shutdown) had throws Exception removed; CHANGELOG updated.

Changes

Cohort / File(s)	Change Summary
Test Framework Utility test/framework/src/main/java/org/opensearch/http/TestDispatcherBuilder.java	New fluent builder for creating HttpServerTransport.Dispatcher with defaults and overridable hooks: dispatchRequest, dispatchBadRequest, dispatchHandler. Adds factory dispatcherBuilderWithDefaults() and nested functional interfaces.
Netty4 Transport Tests modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java, modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java, modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java	Replaced anonymous Dispatcher implementations with TestDispatcherBuilder usage and lambdas; removed unused imports; removed throws Exception from setup()/shutdown() signatures.
ReactorNetty4 Transport Tests plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java, plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java, plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java, plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java	Converted per-test anonymous Dispatcher classes to builder-based dispatchers (including streaming wiring), removed unused imports, and removed throws Exception from lifecycle methods.
Base HTTP Server Tests server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java	Switched anonymous Dispatcher instances to builder-based configuration in affected tests.
Changelog CHANGELOG.md	Added Unreleased 3.x entry noting cleanup of HttpServerTransport.Dispatcher in Netty tests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Review TestDispatcherBuilder.java for correctness of default behaviors and concurrency/thread-context handling.
• Spot-check representative converted tests (Netty4, ReactorNetty4 streaming, and secure variants) to ensure semantics match prior anonymous-dispatcher behavior.
• Verify removal of throws Exception on lifecycle methods is safe for test frameworks used.

Suggested labels

:\:test, \Other`, `enhancement``

Suggested reviewers

• peternied
• reta
• cwperks

Poem

🐰 I stitched a tiny dispatcher with care,
Lambdas for pockets and defaults to share,
Tests hop along tidy, no anonymous mess,
setup and shutdown now simple and less,
A rabbit's small refactor — neat, quick, and fair! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 10.99% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly describes the main change: introducing TestDispatcherBuilder to simplify HttpServerTransport.Dispatcher creation in Netty tests.
Description check	✅ Passed	The description explains the purpose of TestDispatcherBuilder, mentions minor cleanups like removing stale throws clauses, and includes completed checklist items. Description is clear and complete.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

CHANGELOG.md (1)

75-75: Consider adding PR reference for consistency

Nearby changelog entries all link the corresponding PR/issue; adding ([#20160](...)) here would keep the section consistent and traceable, but is optional.

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

28-30: Optionally mark the builder class final

Since this is a focused test utility not intended for extension, you could make the class final to prevent accidental subclassing in tests:

-public class TestDispatcherBuilder {
+public final class TestDispatcherBuilder {

Purely optional, but it better communicates intent and avoids surprising inheritance chains in test code.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b7f013f and 7ada4c7.

📒 Files selected for processing (10)

• CHANGELOG.md (1 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (1 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (6 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (11 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (1 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (3 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java (8 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (8 hunks)

🧰 Additional context used

🧬 Code graph analysis (7)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (2)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (2)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (1)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (1)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (1)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)

• GitHub Check: gradle-check
• GitHub Check: precommit (21, windows-2025, true)
• GitHub Check: precommit (21, macos-15)
• GitHub Check: precommit (25, macos-15)
• GitHub Check: Analyze (java)
• GitHub Check: precommit (21, ubuntu-latest)
• GitHub Check: precommit (25, macos-15-intel)
• GitHub Check: precommit (21, ubuntu-24.04-arm)
• GitHub Check: precommit (25, ubuntu-24.04-arm)
• GitHub Check: precommit (25, windows-latest)
• GitHub Check: precommit (21, windows-latest)
• GitHub Check: detect-breaking-change
• GitHub Check: precommit (25, ubuntu-latest)
• GitHub Check: precommit (21, macos-15-intel)
• GitHub Check: assemble (25, ubuntu-24.04-arm)
• GitHub Check: assemble (25, windows-latest)
• GitHub Check: assemble (25, ubuntu-latest)
• GitHub Check: assemble (21, ubuntu-24.04-arm)
• GitHub Check: assemble (21, ubuntu-latest)
• GitHub Check: assemble (21, windows-latest)
• GitHub Check: Mend Security Check

🔇 Additional comments (45)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (3)

73-78: Non-throwing setup is fine

setup() does not throw checked exceptions, so dropping throws Exception matches the implementation and keeps the signature clean.

---

80-83: Non-throwing shutdown is appropriate

shutdown() body cannot throw checked exceptions; removing throws Exception is correct and consistent with other tests.

---

85-96: Dispatcher builder usage preserves bad‑request behavior

Using TestDispatcherBuilder.withDefaults() with a failing dispatchRequest and custom dispatchBadRequest mirrors the old anonymous Dispatcher while centralizing the common wiring; looks good.

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (3)

50-55: setup signature cleanup is correct

No checked exceptions are thrown here, so removing throws Exception is safe and simplifies the test.

---

57-60: shutdown signature cleanup is correct

Dropping throws Exception matches the implementation and other tests.

---

62-73: Refactored dispatcher matches previous bad‑request handling

The TestDispatcherBuilder configuration (failing normal requests, custom dispatchBadRequest) keeps the original test intent while reducing boilerplate.

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (8)

32-32: Importing TestDispatcherBuilder aligns secure tests with shared helper

Pulling in the builder here is consistent with other Netty/Reactor tests and centralizes Dispatcher setup.

---

174-183: shutdown signature change is benign

The method only does null checks and shutdowns; removing throws Exception is appropriate.

---

185-205: Javadoc formatting only

Only spacing/blank-line adjustments in Javadoc; behavior is unaffected.

---

221-224: Expect‑header dispatcher correctly uses builder

Using the builder with a simple OK BytesRestResponse("done") preserves the original expect/continue behavior while leveraging shared defaults.

---

318-328: Bad‑request dispatcher mirrors non‑secure variant

The custom withDispatchBadRequest that records cause and sends a BAD_REQUEST response matches the non-secure Netty test and keeps the assertion that a TooLongFrameException occurred.

---

381-390: Large compressed response dispatcher preserved via builder

The lambda guarding on url and returning a large OK body is unchanged in logic; using the builder just removes anonymous class noise.

---

425-425: CORS tests still rely on pre‑dispatch handling

Using TestDispatcherBuilder.withDefaults().build() here keeps the “no application handler should be hit” invariant similar to NullDispatcher; CORS logic remains in the transport layer.

---

479-481: Read‑timeout test now uses shared dispatcher helper

Using the default builder dispatcher is consistent with other timeout tests and keeps all actual behavior in the transport.

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (7)

115-121: setup no longer declaring throws Exception is appropriate

Method body doesn’t throw checked exceptions; signature cleanup is correct.

---

123-132: shutdown cleanup is safe

Explicit shutdown and nulling of fields don’t require a checked exception; updated signature is fine.

---

173-179: Builder‑based dispatcher for expect‑header tests looks good

The dispatcher sending "done" via BytesRestResponse matches prior behavior while using the shared TestDispatcherBuilder.

---

269-280: Bad‑request dispatcher correctly focuses on dispatchBadRequest

Overriding only withDispatchBadRequest and using the builder defaults elsewhere clearly expresses the test’s intent and keeps the cause capture semantics.

---

332-341: Large compressed response dispatcher refactor is behavior‑preserving

The url guard and large OK body logic are unchanged; moving to the builder simply standardizes Dispatcher construction.

---

391-393: CORS tests leveraging default dispatcher is consistent

Using withDefaults().build() here mirrors prior NullDispatcher behavior: CORS handling occurs before any app dispatch.

---

445-447: Read‑timeout test now uses shared dispatcher helper

Default dispatcher from TestDispatcherBuilder is sufficient since the test only cares about connection closure; change is straightforward.

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (9)

31-31: Shared dispatcher helper imported for secure Netty tests

Adding TestDispatcherBuilder here brings the secure tests in line with the rest of the Netty test suite.

---

145-163: setup signature simplification is correct

Initialization does not throw checked exceptions; dropping throws Exception matches the implementation.

---

165-174: shutdown cleanup is appropriate

The teardown logic is unchanged; removing throws Exception is safe.

---

176-207: Javadoc spacing only

Only blank-line formatting in the Javadocs changed; no behavioral impact.

---

218-224: Expect‑header dispatcher correctly refactored to builder

The dispatcher’s behavior (sending "done" on success) stays the same while using the central TestDispatcherBuilder.

---

318-328: Bad‑request dispatcher logic preserved

Using withDispatchBadRequest to send a BAD_REQUEST with a custom message and track causeReference matches the previous anonymous implementation.

---

381-390: Large compressed response handler via builder is equivalent

URL check plus OK response are unchanged; builder usage simply standardizes construction across tests.

---

425-425: CORS dispatcher via withDefaults() is consistent with other tests

Relying on transport-level CORS logic with a default dispatcher keeps app-level handlers out of the path as before.

---

479-481: Read‑timeout test uses shared dispatcher pattern

Default dispatcher from the builder is sufficient here; behavior remains focused on timeout-induced close.

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java (9)

109-115: setup signature cleaned up correctly

No checked exceptions thrown; removing throws Exception is appropriate.

---

117-126: shutdown signature cleaned up correctly

Teardown is unchanged functionally; non-throwing signature is fine.

---

170-176: Expect‑header dispatcher via builder mirrors Netty variant

The dispatcher responding with "done" now uses TestDispatcherBuilder, matching the non-Reactor tests and keeping semantics the same.

---

257-258: Bad‑request test using default dispatcher is intentional

Using withDefaults().build() ensures any unexpected dispatch would fail the test, while URI length errors are handled at the transport layer as before.

---

301-305: Dispatch‑failed test refactor preserves 500 behavior

The custom withDispatchRequest throwing RuntimeException still drives the INTERNAL_SERVER_ERROR expectations; builder just removes boilerplate.

---

341-350: Large compressed response dispatcher refactor is behavior‑preserving

URL guard and body content checks remain identical; dispatcher is now built via the shared helper.

---

401-410: Connections‑closed test dispatcher refactor looks good

The dispatcher still serves only the expected URL and asserts on others; the looped request/response and stats checks remain unchanged.

---

446-448: CORS test using default dispatcher keeps focus on transport CORS logic

withDefaults().build() continues to ensure no app handler is needed; behavior matches other CORS tests.

---

500-502: Connect‑timeout test uses shared dispatcher helper correctly

Default dispatcher suffices; the test still validates timeout via channel closure.

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (4)

63-68: Lambda simplification is fine

Changing (str) -> to str -> is a pure style tweak with no behavioral impact; XCONTENT_CONVERTER remains equivalent.

---

75-81: setup signature cleanup is appropriate

Initialization logic doesn’t throw checked exceptions; non-throwing signature is correct.

---

83-92: shutdown signature cleanup is appropriate

Teardown logic is unchanged; removing throws Exception is safe and consistent with other tests.

---

177-216: Streaming dispatcher refactor preserves behavior and clarifies responsibilities

Using TestDispatcherBuilder to:

• advertise a streaming RestHandler via withDispatchHandler (supportsStreaming=true, handleRequest guarded to fail if ever called), and
• implement the actual streaming pipeline in withDispatchRequest (asserting StreamingRestChannel, waiting for writability, emitting chunks on the WRITE executor, and closing bytesOutput if Releasable)

keeps the original streaming semantics while centralizing Dispatcher construction. The URL guard and chunking logic remain unchanged.

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

24-86: Well-factored test dispatcher helper; defaults and delegation look correct

The builder defaults (logging + AssertionError) and the build() implementation cleanly delegate to the configured lambdas and should make dispatcher setup in tests much clearer. No issues from a correctness or test-behavior perspective.

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

24-86: Dispatcher builder implementation is correct and consistent with reactor-netty4 version

The builder’s defaults and build() delegation are sound and match the reactor-netty4 counterpart, giving consistent test behavior across modules. No functional issues spotted.

[github-actions]

❌ Gradle check result for e980258: FAILURE

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?

[fdesu]

Seems like a flaky test failure

> Task :server:internalClusterTest

Tests with failures:
 - org.opensearch.indices.replication.WarmIndexSegmentReplicationIT.testNodeDropWithOngoingReplication

5584 tests completed, 1 failed, 63 skipped

> Task :server:internalClusterTest FAILED

[github-actions]

✅ Gradle check result for 82bdb8c: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.23%. Comparing base (930ae74) to head (8d9a890).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20160      +/-   ##
============================================
+ Coverage     73.20%   73.23%   +0.03%     
- Complexity    71745    71803      +58     
============================================
  Files          5795     5795              
  Lines        328304   328298       -6     
  Branches      47283    47279       -4     
============================================
+ Hits         240334   240433      +99     
+ Misses        68663    68595      -68     
+ Partials      19307    19270      -37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

❌ Gradle check result for 7e8b64f: FAILURE

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?

[github-actions]

✅ Gradle check result for f95862f: SUCCESS

[github-actions]

❌ Gradle check result for e896324: FAILURE

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?

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e896324 and fa4f49e.

📒 Files selected for processing (1)

• CHANGELOG.md (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.

🪛 LanguageTool

CHANGELOG.md

[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ontroller with org.opensearch.secure_sm.AccessController in discovery plugins ([#19802](https://...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[style] ~31-~31: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....Stats and RemoteSegmentTransferTracker.Stats class to use the Builder pattern instea...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)

• GitHub Check: gradle-check
• GitHub Check: precommit (21, macos-15-intel)
• GitHub Check: precommit (25, macos-15-intel)
• GitHub Check: precommit (25, windows-latest)
• GitHub Check: precommit (21, ubuntu-24.04-arm)
• GitHub Check: precommit (25, ubuntu-24.04-arm)
• GitHub Check: precommit (21, windows-latest)
• GitHub Check: precommit (25, macos-15)
• GitHub Check: precommit (21, ubuntu-latest)
• GitHub Check: precommit (21, windows-2025, true)
• GitHub Check: precommit (21, macos-15)
• GitHub Check: precommit (25, ubuntu-latest)
• GitHub Check: assemble (25, ubuntu-latest)
• GitHub Check: assemble (25, ubuntu-24.04-arm)
• GitHub Check: assemble (21, ubuntu-24.04-arm)
• GitHub Check: assemble (21, windows-latest)
• GitHub Check: assemble (21, ubuntu-latest)
• GitHub Check: assemble (25, windows-latest)
• GitHub Check: detect-breaking-change
• GitHub Check: Analyze (java)
• GitHub Check: Mend Security Check

[fdesu]

@reta would you be able to take a look whenever time permits? This change is a tiny bit of polishing of netty-related testing code. Hope it makes it easier to maintain and add more testing further.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (3)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

49-62: Fail fast on null callbacks (and enforce non-null Optional result).
Passing null to withDispatch* will currently fail later with a less actionable NPE; also consider guarding against a null return from DispatchHandler.

+import java.util.Objects;
@@
 public TestDispatcherBuilder withDispatchRequest(DispatchRequest dispatchRequest) {
-    this.dispatchRequest = dispatchRequest;
+    this.dispatchRequest = Objects.requireNonNull(dispatchRequest, "dispatchRequest");
     return this;
 }
@@
 public TestDispatcherBuilder withDispatchBadRequest(DispatchBadRequest dispatchBadRequest) {
-    this.dispatchBadRequest = dispatchBadRequest;
+    this.dispatchBadRequest = Objects.requireNonNull(dispatchBadRequest, "dispatchBadRequest");
     return this;
 }
@@
 public TestDispatcherBuilder withDispatchHandler(DispatchHandler dispatchHandler) {
-    this.dispatchHandler = dispatchHandler;
+    this.dispatchHandler = Objects.requireNonNull(dispatchHandler, "dispatchHandler");
     return this;
 }
@@
         ) {
-            return dispatchHandler.dispatchHandler(uri, rawPath, method, params);
+            return Objects.requireNonNull(
+                dispatchHandler.dispatchHandler(uri, rawPath, method, params),
+                "dispatchHandler returned null"
+            );
         }

Also applies to: 98-101

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (2)

49-62: Fail fast on null callbacks (and enforce non-null Optional result).
Same recommendation as the reactor-netty4 variant.

+import java.util.Objects;
@@
 public TestDispatcherBuilder withDispatchRequest(DispatchRequest dispatchRequest) {
-    this.dispatchRequest = dispatchRequest;
+    this.dispatchRequest = Objects.requireNonNull(dispatchRequest, "dispatchRequest");
     return this;
 }
@@
 public TestDispatcherBuilder withDispatchBadRequest(DispatchBadRequest dispatchBadRequest) {
-    this.dispatchBadRequest = dispatchBadRequest;
+    this.dispatchBadRequest = Objects.requireNonNull(dispatchBadRequest, "dispatchBadRequest");
     return this;
 }
@@
 public TestDispatcherBuilder withDispatchHandler(DispatchHandler dispatchHandler) {
-    this.dispatchHandler = dispatchHandler;
+    this.dispatchHandler = Objects.requireNonNull(dispatchHandler, "dispatchHandler");
     return this;
 }
@@
         ) {
-            return dispatchHandler.dispatchHandler(uri, rawPath, method, params);
+            return Objects.requireNonNull(
+                dispatchHandler.dispatchHandler(uri, rawPath, method, params),
+                "dispatchHandler returned null"
+            );
         }

Also applies to: 98-101

---

24-103: Two near-identical builders: consider a shared test-fixture to prevent drift.
If these must stay duplicated due to module boundaries, it may still be worth a short comment in both files pointing to the “paired” implementation to keep changes synchronized.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fa4f49e and fcf389d.

📒 Files selected for processing (10)

• CHANGELOG.md (1 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (1 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (6 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (11 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (1 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (3 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java (8 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (8 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.

📚 Learning: 2025-12-12T18:40:01.303Z

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

Applied to files:

• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java

📚 Learning: 2025-12-12T13:31:41.214Z

Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.

Applied to files:

• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java

🧬 Code graph analysis (6)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (1)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (2)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (1)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (2)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

🪛 LanguageTool

CHANGELOG.md

[grammar] ~14-~14: Use a hyphen to join words.
Context: .../pull/19539)) - Add a mapper for context aware segments grouping criteria ([#1923...

(QB_NEW_EN_HYPHEN)

---

[uncategorized] ~16-~16: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...568)) - Add support for repository with Server side encryption enabled and client side encr...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

---

[grammar] ~16-~16: Use a hyphen to join words.
Context: ...erver side encryption enabled and client side encryption as well based on a flag....

(QB_NEW_EN_HYPHEN)

---

[uncategorized] ~33-~33: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...enSearch/pull/19878)) - Add support for context aware segments ([#19098](https://github.com/o...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

---

[style] ~59-~59: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ontroller with org.opensearch.secure_sm.AccessController in discovery plugins ([#19802](https://...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[style] ~67-~67: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....Stats and RemoteSegmentTransferTracker.Stats class to use the Builder pattern instea...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[uncategorized] ~116-~116: The official name of this software platform is spelled with a capital “H”.
Context: ...-project/OpenSearch/pull/19692)) - Bump com.github.spotbugs:spotbugs-annotations from 4.9...

(GITHUB)

---

[uncategorized] ~119-~119: The official name of this software platform is spelled with a capital “H”.
Context: ...-project/OpenSearch/pull/19763)) - Bump github/codeql-action from 3 to 4 ([#19785](ht...

(GITHUB)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)

• GitHub Check: gradle-check
• GitHub Check: precommit (21, windows-2025, true)
• GitHub Check: precommit (25, windows-latest)
• GitHub Check: precommit (21, macos-15-intel)
• GitHub Check: precommit (25, macos-15-intel)
• GitHub Check: precommit (21, macos-15)
• GitHub Check: precommit (25, macos-15)
• GitHub Check: precommit (21, ubuntu-latest)
• GitHub Check: precommit (21, ubuntu-24.04-arm)
• GitHub Check: precommit (21, windows-latest)
• GitHub Check: assemble (25, ubuntu-latest)
• GitHub Check: assemble (21, ubuntu-latest)
• GitHub Check: assemble (21, windows-latest)
• GitHub Check: assemble (25, ubuntu-24.04-arm)
• GitHub Check: assemble (25, windows-latest)
• GitHub Check: assemble (21, ubuntu-24.04-arm)
• GitHub Check: Analyze (java)

🔇 Additional comments (17)

CHANGELOG.md (1)

81-81: LGTM - Test infrastructure improvement appropriately documented.

The CHANGELOG entry accurately describes the test cleanup. While a previous review questioned whether test-only changes should appear in the main CHANGELOG, documenting test infrastructure improvements can help maintainers understand the evolution of the test suite.

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (2)

74-83: LGTM - Lifecycle methods cleaned up.

Removing throws Exception from setup() and shutdown() is a good cleanup since these methods don't throw checked exceptions.

---

86-96: LGTM - Excellent refactoring with TestDispatcherBuilder.

The migration from anonymous dispatcher to the builder pattern improves maintainability and provides consistent default behaviors across tests. The lambda-based dispatch configuration is cleaner and more expressive than the previous anonymous class approach.

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (2)

51-60: LGTM - Lifecycle methods cleaned up.

Consistent with the changes in the netty4 module, removing unnecessary exception declarations.

---

63-73: LGTM - Builder pattern applied consistently.

The TestDispatcherBuilder is applied consistently across both netty4 and reactor-netty4 test suites, promoting uniformity in test infrastructure.

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (3)

146-174: LGTM - Lifecycle cleanup in secure transport tests.

Consistent removal of exception declarations from setup() and shutdown() methods.

---

218-224: LGTM - Builder pattern improves test readability.

The multiline lambda format in withDispatchRequest makes the test intent clearer compared to nested anonymous classes. The dispatcher immediately shows it will send a simple "done" response.

---

318-328: LGTM - Consistent builder usage across test methods.

All test methods now use the same TestDispatcherBuilder pattern, making the test suite more uniform and easier to maintain.

Also applies to: 381-390

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (2)

175-183: LGTM - Appropriate exception handling in lifecycle methods.

Note that setup() correctly retains throws Exception (line 109) because KeyStoreUtils.createServerKeyStore() can throw checked exceptions. Only shutdown() has the exception declaration removed, which is correct.

---

223-229: LGTM - Builder pattern applied consistently across reactor-netty4 secure tests.

The TestDispatcherBuilder integration is consistent across all test methods, with appropriate customization per test scenario (basic dispatching, bad requests, exception handling, streaming).

Also applies to: 307-307, 353-357

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (2)

116-132: LGTM - Clean lifecycle methods.

Consistent with other test files in removing unnecessary exception declarations.

---

173-179: LGTM - Builder pattern enhances test suite uniformity.

The TestDispatcherBuilder creates a consistent testing pattern across the entire netty4 test suite. Tests are now easier to understand and maintain, with clear separation between default behaviors and test-specific customizations.

Also applies to: 270-280, 332-341

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java (2)

110-126: LGTM - Lifecycle cleanup in reactor transport tests.

Consistent application of the lifecycle improvements.

---

170-176: LGTM - Comprehensive builder adoption across reactor-netty4 tests.

All test scenarios (expect headers, bad requests, dispatch failures, large responses, CORS, timeouts) now use the TestDispatcherBuilder, creating a unified test infrastructure across both netty4 and reactor-netty4 modules.

Also applies to: 257-257, 301-305, 341-350

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (2)

76-92: LGTM - Lifecycle cleanup in streaming tests.

Consistent with all other test files.

---

178-216: LGTM - Builder pattern handles complex streaming scenarios elegantly.

The TestDispatcherBuilder successfully accommodates the more complex streaming test scenario by allowing customization of both dispatchHandler (to return a streaming-capable RestHandler) and dispatchRequest (to handle the actual streaming logic). This demonstrates the flexibility of the builder approach while maintaining clear test structure.

The streaming logic itself (Flux concatenation, channel writability checks, thread pool usage) remains unchanged and correct.

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

24-103: Clean, consistent test helper; defaults are sensible.
Centralizing “unexpected request” logging + assertion failure should make test intent and failures clearer.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (3)

CHANGELOG.md (1)

13-48: Fix small changelog text issues + reconsider test-only entry placement.

• Line 13: missing space before the PR link: sub aggregation([#19573]...) → sub aggregation ([#19573]...).
• Line 17: “s3” → “S3” for consistency with common acronym casing.
• Line 42: IndexPressutreStats looks like a typo (likely IndexPressureStats); please confirm and correct.
• Lines 23 / 26: trailing periods are inconsistent with most bullets—worth normalizing in this block.
• Line 48: since this is a test-only cleanup, please double-check whether it belongs in the top-level “Changed” release notes per project convention, or should be omitted/moved to a testing/dev-notes section (if you have one).

- - Combining filter rewrite and skip list to optimize sub aggregation([#19573](https://github.com/opensearch-project/OpenSearch/pull/19573))
+ - Combining filter rewrite and skip list to optimize sub aggregation ([#19573](https://github.com/opensearch-project/OpenSearch/pull/19573))
...
- - Onboarding new maven snapshots publishing to s3 ([#19619](https://github.com/opensearch-project/OpenSearch/pull/19619))
+ - Onboarding new maven snapshots publishing to S3 ([#19619](https://github.com/opensearch-project/OpenSearch/pull/19619))
...
- - Refactor the IndexPressutreStats, DeviceStats and TransportStats class to use the Builder pattern instead of constructors ([#19991](https://github.com/opensearch-project/OpenSearch/pull/19991))
+ - Refactor the IndexPressureStats, DeviceStats and TransportStats class to use the Builder pattern instead of constructors ([#19991](https://github.com/opensearch-project/OpenSearch/pull/19991))

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

64-86: Snapshot builder state in build() to prevent cross-test mutation.

The anonymous dispatcher references the builder's mutable fields directly. If a builder is reused or mutated after build(), previously built dispatchers will change behavior, potentially causing flaky tests.

Apply this fix to capture the current state:

 public HttpServerTransport.Dispatcher build() {
+    final DispatchRequest dispatchRequest = this.dispatchRequest;
+    final DispatchBadRequest dispatchBadRequest = this.dispatchBadRequest;
+    final DispatchHandler dispatchHandler = this.dispatchHandler;
     return new HttpServerTransport.Dispatcher() {
         @Override
         public Optional<RestHandler> dispatchHandler(
             String uri,
             String rawPath,
             RestRequest.Method method,
             Map<String, String> params
         ) {
             return dispatchHandler.dispatchHandler(uri, rawPath, method, params);
         }

         @Override
         public void dispatchRequest(RestRequest request, RestChannel channel, ThreadContext threadContext) {
             dispatchRequest.dispatchRequest(request, channel, threadContext);
         }

         @Override
         public void dispatchBadRequest(RestChannel channel, ThreadContext threadContext, Throwable cause) {
             dispatchBadRequest.dispatchBadRequest(channel, threadContext, cause);
         }
     };
 }

Note: This same issue exists in the Netty4 variant at modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (lines 64-86) and should be fixed there as well.

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

32-85: Snapshot builder state in build() to avoid surprising cross-test mutation/flakes.

The returned Dispatcher closes over mutable builder fields (dispatchHandler, dispatchRequest, dispatchBadRequest). If a test reuses the builder and mutates it after build(), previously built dispatchers can silently change behavior.

 public HttpServerTransport.Dispatcher build() {
+    final DispatchRequest dispatchRequest = this.dispatchRequest;
+    final DispatchBadRequest dispatchBadRequest = this.dispatchBadRequest;
+    final DispatchHandler dispatchHandler = this.dispatchHandler;
     return new HttpServerTransport.Dispatcher() {
         @Override
         public Optional<RestHandler> dispatchHandler(
             String uri,
             String rawPath,
             RestRequest.Method method,
             Map<String, String> params
         ) {
             return dispatchHandler.dispatchHandler(uri, rawPath, method, params);
         }

         @Override
         public void dispatchRequest(RestRequest request, RestChannel channel, ThreadContext threadContext) {
             dispatchRequest.dispatchRequest(request, channel, threadContext);
         }

         @Override
         public void dispatchBadRequest(RestChannel channel, ThreadContext threadContext, Throwable cause) {
             dispatchBadRequest.dispatchBadRequest(channel, threadContext, cause);
         }
     };
 }

🧹 Nitpick comments (2)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (1)

63-68: LGTM! Minor lambda syntax cleanup.

Removing unnecessary parentheses around single lambda parameter improves consistency with Java conventions.

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

49-62: Fail fast on null callbacks in withDispatch* setters.

Today withDispatchRequest(null) (etc.) will defer failure to dispatch time via NPE, which is harder to debug in tests.

+import java.util.Objects;
 ...
 public TestDispatcherBuilder withDispatchRequest(DispatchRequest dispatchRequest) {
-    this.dispatchRequest = dispatchRequest;
+    this.dispatchRequest = Objects.requireNonNull(dispatchRequest, "dispatchRequest");
     return this;
 }

 public TestDispatcherBuilder withDispatchBadRequest(DispatchBadRequest dispatchBadRequest) {
-    this.dispatchBadRequest = dispatchBadRequest;
+    this.dispatchBadRequest = Objects.requireNonNull(dispatchBadRequest, "dispatchBadRequest");
     return this;
 }

 public TestDispatcherBuilder withDispatchHandler(DispatchHandler dispatchHandler) {
-    this.dispatchHandler = dispatchHandler;
+    this.dispatchHandler = Objects.requireNonNull(dispatchHandler, "dispatchHandler");
     return this;
 }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fcf389d and 3f4e68b.

📒 Files selected for processing (10)

• CHANGELOG.md (1 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (1 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (6 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (11 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (1 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (3 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java (8 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (8 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

📚 Learning: 2025-12-12T18:40:01.303Z

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

Applied to files:

• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java

📚 Learning: 2025-12-12T13:31:41.214Z

Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.

Applied to files:

• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java

🧬 Code graph analysis (7)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (2)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (1)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (1)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (1)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (1)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (1)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-103)

🪛 LanguageTool

CHANGELOG.md

[style] ~25-~25: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ontroller with org.opensearch.secure_sm.AccessController in discovery plugins ([#19802](https://...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[style] ~33-~33: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....Stats and RemoteSegmentTransferTracker.Stats class to use the Builder pattern instea...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)

• GitHub Check: gradle-check
• GitHub Check: detect-breaking-change
• GitHub Check: precommit (25, macos-15-intel)
• GitHub Check: precommit (21, ubuntu-24.04-arm)
• GitHub Check: precommit (25, macos-15)
• GitHub Check: precommit (25, windows-latest)
• GitHub Check: precommit (21, macos-15-intel)
• GitHub Check: precommit (21, macos-15)
• GitHub Check: precommit (21, windows-latest)
• GitHub Check: precommit (21, windows-2025, true)
• GitHub Check: precommit (21, ubuntu-latest)
• GitHub Check: precommit (25, ubuntu-latest)
• GitHub Check: Analyze (java)
• GitHub Check: assemble (25, ubuntu-latest)
• GitHub Check: assemble (21, ubuntu-24.04-arm)
• GitHub Check: assemble (25, windows-latest)
• GitHub Check: assemble (21, ubuntu-latest)
• GitHub Check: assemble (25, ubuntu-24.04-arm)
• GitHub Check: assemble (21, windows-latest)

🔇 Additional comments (17)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (2)

74-83: LGTM! Lifecycle method signatures simplified.

Removing throws Exception from setup() and shutdown() is appropriate since neither method throws checked exceptions.

---

86-96: LGTM! Builder pattern improves test readability.

The refactor from anonymous dispatcher to TestDispatcherBuilder makes the test setup clearer and more maintainable while preserving the original test behavior.

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (2)

51-60: LGTM! Consistent lifecycle method cleanup.

The removal of throws Exception is consistent with the pattern applied across all test files in this PR.

---

63-73: LGTM! Builder pattern applied consistently.

The TestDispatcherBuilder usage mirrors the implementation in the Netty4 module, maintaining consistency across the codebase.

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (3)

175-183: LGTM! Lifecycle cleanup.

Removing throws clause is safe and appropriate.

---

221-229: LGTM! Dispatcher construction improved.

The builder pattern consolidates dispatcher setup, making the test helper method cleaner and easier to understand.

---

307-307: LGTM! Consistent builder adoption across test methods.

All test methods now use TestDispatcherBuilder consistently, improving maintainability while preserving test coverage.

Also applies to: 353-357, 394-403, 453-453, 508-508

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (2)

116-132: LGTM! Lifecycle method cleanup.

Consistent with the pattern applied across all test files.

---

173-179: LGTM! Comprehensive builder adoption.

All test methods in this class now consistently use the TestDispatcherBuilder pattern, improving code quality without affecting test coverage.

Also applies to: 270-280, 332-341, 392-392, 446-446

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (2)

146-174: LGTM! Lifecycle and dispatcher construction improvements.

The changes are consistent with the pattern applied across all other test files, improving maintainability.

Also applies to: 218-224

---

318-328: LGTM! Builder pattern applied consistently.

All test methods now use TestDispatcherBuilder, maintaining consistency across the secure Netty4 transport tests.

Also applies to: 381-390, 425-425, 480-480

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java (2)

110-126: LGTM! Lifecycle method cleanup.

Consistent with the refactoring pattern across all test files.

---

170-176: LGTM! Comprehensive builder adoption across all test methods.

The TestDispatcherBuilder is now used consistently throughout this test class, improving readability and maintainability.

Also applies to: 257-257, 301-305, 341-350, 401-410, 447-447, 501-501

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (2)

76-92: LGTM! Lifecycle method cleanup.

Consistent with the pattern applied across all test files.

---

178-216: LGTM! Builder pattern applied to streaming dispatcher.

The refactor successfully preserves the complex streaming logic while adopting the builder pattern for consistency. The streaming-specific handler is properly configured via withDispatchHandler() and withDispatchRequest().

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (2)

28-62: LGTM! Clean builder API with sensible test defaults.

The fluent API provides an intuitive way to configure dispatchers for tests. The default implementations (logging + AssertionError) are appropriate for catching unexpected behavior in tests.

---

88-101: LGTM! Clean functional interfaces.

The three interfaces provide clear contracts for customizing dispatcher behavior. Using @FunctionalInterface is appropriate.

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3f4e68b and 084ab2f.

📒 Files selected for processing (2)

• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.

📚 Learning: 2025-12-12T18:40:01.303Z

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

Applied to files:

• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java

📚 Learning: 2025-12-12T13:31:41.214Z

Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.

Applied to files:

• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java

🧬 Code graph analysis (2)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-106)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-106)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)

• GitHub Check: gradle-check
• GitHub Check: precommit (25, ubuntu-latest)
• GitHub Check: precommit (25, windows-latest)
• GitHub Check: precommit (25, macos-15-intel)
• GitHub Check: precommit (21, ubuntu-latest)
• GitHub Check: precommit (21, windows-2025, true)
• GitHub Check: precommit (21, macos-15)
• GitHub Check: precommit (25, macos-15)
• GitHub Check: precommit (21, windows-latest)
• GitHub Check: precommit (21, ubuntu-24.04-arm)
• GitHub Check: precommit (25, ubuntu-24.04-arm)
• GitHub Check: precommit (21, macos-15-intel)
• GitHub Check: assemble (25, windows-latest)
• GitHub Check: Analyze (java)
• GitHub Check: detect-breaking-change
• GitHub Check: assemble (25, ubuntu-24.04-arm)
• GitHub Check: assemble (25, ubuntu-latest)
• GitHub Check: assemble (21, ubuntu-latest)
• GitHub Check: assemble (21, ubuntu-24.04-arm)
• GitHub Check: assemble (21, windows-latest)

🔇 Additional comments (4)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (4)

1-22: LGTM!

License header, package declaration, and imports are correct and properly organized.

---

24-43: Well-designed fail-fast defaults for test utilities.

The default behaviors appropriately surface unexpected dispatch calls with detailed logging and immediate assertion failures, which is ideal for catching test setup issues early.

---

45-62: LGTM!

Clean fluent builder API that follows standard patterns.

---

91-106: LGTM!

Clean functional interface definitions that enable concise lambda-based test configurations.

[github-actions]

❌ Gradle check result for 084ab2f: FAILURE

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?

[reta]

@reta would you be able to take a look whenever time permits? This change is a tiny bit of polishing of netty-related testing code. Hope it makes it easier to maintain and add more testing further.

Thanks @fdesu , quick question, beside just refactoring (or polishing as you said), any particular problem(s) you run into and this change helps you to address? Yes, the test cases are relying on quite a few duplicated pieces, but I would generally lean towards "we had/have a problem, refactoring the test case would help us the not have this problem anymore". Thank you.

[fdesu]

Seems like a failure in spotless in unrelated module, I guess rerunning CI should do the trick

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':libs:opensearch-nio:spotlessJava'.
> Cannot fingerprint input property 'stepsInternalEquality': value 'com.diffplug.spotless.ConfigurationCacheHackList@2154da26' cannot be serialized.

[fdesu]

@reta fair question, there was no issue with running tests or failures of any sorts. While adding additional testing in another PR, I was going through existing test suites to see how it’s done and had just a tiny bit of trouble to distinguish the setup code/preconditions from actual code under test.

So, my hope here was - with clear naming, concise lambdas and centralized DSL to highlight it very clearly - this is setup code. Also I wanted to remove duplications and refactor a bit the visual noise to help focus on code under test.

So, this PR is really about reducing mental complexity and helping to focus on the code we’re currently testing.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

64-88: Make captured locals final + fix builderDispatcherHandler naming.
The snapshot approach is great; final improves intent. The handler local name typo has been flagged before.

     public HttpServerTransport.Dispatcher build() {
-        DispatchRequest builderDispatchRequest = this.dispatchRequest;
-        DispatchBadRequest builderDispatchBadRequest = this.dispatchBadRequest;
-        DispatchHandler builderDispatcherHandler = this.dispatchHandler;
+        final DispatchRequest builderDispatchRequest = this.dispatchRequest;
+        final DispatchBadRequest builderDispatchBadRequest = this.dispatchBadRequest;
+        final DispatchHandler builderDispatchHandler = this.dispatchHandler;
         return new HttpServerTransport.Dispatcher() {
@@
             ) {
-                return builderDispatcherHandler.dispatchHandler(uri, rawPath, method, params);
+                return builderDispatchHandler.dispatchHandler(uri, rawPath, method, params);
             }

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

67-67: Fix typo: builderDispatcherHandler → builderDispatchHandler.

The variable name builderDispatcherHandler on line 67 contains an extra "er" and is inconsistent with the naming pattern used for builderDispatchRequest and builderDispatchBadRequest. This was flagged in a previous review but remains unfixed.

Apply this diff to correct the variable name:

-        DispatchHandler builderDispatcherHandler = this.dispatchHandler;
+        DispatchHandler builderDispatchHandler = this.dispatchHandler;

Also update the usage at line 76:

-                return builderDispatcherHandler.dispatchHandler(uri, rawPath, method, params);
+                return builderDispatchHandler.dispatchHandler(uri, rawPath, method, params);

Note: The same typo exists in modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java and should be fixed there as well for consistency.

🧹 Nitpick comments (1)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

49-62: Fail fast on null callbacks in withDispatch* (clearer test failures).

+import java.util.Objects;
@@
     public TestDispatcherBuilder withDispatchRequest(DispatchRequest dispatchRequest) {
-        this.dispatchRequest = dispatchRequest;
+        this.dispatchRequest = Objects.requireNonNull(dispatchRequest, "dispatchRequest");
         return this;
     }
@@
     public TestDispatcherBuilder withDispatchBadRequest(DispatchBadRequest dispatchBadRequest) {
-        this.dispatchBadRequest = dispatchBadRequest;
+        this.dispatchBadRequest = Objects.requireNonNull(dispatchBadRequest, "dispatchBadRequest");
         return this;
     }
@@
     public TestDispatcherBuilder withDispatchHandler(DispatchHandler dispatchHandler) {
-        this.dispatchHandler = dispatchHandler;
+        this.dispatchHandler = Objects.requireNonNull(dispatchHandler, "dispatchHandler");
         return this;
     }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 084ab2f and fb76685.

📒 Files selected for processing (2)

• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.

📚 Learning: 2025-12-12T13:31:41.214Z

Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.

Applied to files:

• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java

📚 Learning: 2025-12-12T18:40:01.303Z

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

Applied to files:

• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java

🧬 Code graph analysis (2)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-106)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/TestDispatcherBuilder.java (1)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (28-106)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)

• GitHub Check: gradle-check
• GitHub Check: precommit (25, macos-15-intel)
• GitHub Check: assemble (25, ubuntu-24.04-arm)
• GitHub Check: assemble (21, ubuntu-latest)
• GitHub Check: assemble (25, ubuntu-latest)
• GitHub Check: assemble (21, ubuntu-24.04-arm)
• GitHub Check: assemble (21, windows-latest)
• GitHub Check: assemble (25, windows-latest)
• GitHub Check: precommit (21, windows-2025, true)
• GitHub Check: precommit (21, ubuntu-latest)
• GitHub Check: precommit (25, ubuntu-latest)
• GitHub Check: precommit (21, macos-15)
• GitHub Check: precommit (25, ubuntu-24.04-arm)
• GitHub Check: precommit (25, windows-latest)
• GitHub Check: precommit (21, ubuntu-24.04-arm)
• GitHub Check: precommit (21, macos-15-intel)
• GitHub Check: precommit (25, macos-15)
• GitHub Check: precommit (21, windows-latest)
• GitHub Check: detect-breaking-change
• GitHub Check: Analyze (java)

🔇 Additional comments (1)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/TestDispatcherBuilder.java (1)

91-104: Functional interfaces look good and appropriately minimal for tests.

[github-actions]

✅ Gradle check result for fb76685: SUCCESS

[reta]

@reta fair question, there was no issue with running tests or failures of any sorts. While adding additional testing in another PR, I was going through existing test suites to see how it’s done and had just a tiny bit of trouble to distinguish the setup code/preconditions from actual code under test.

So, my hope here was - with clear naming, concise lambdas and centralized DSL to highlight it very clearly - this is setup code. Also I wanted to remove duplications and refactor a bit the visual noise to help focus on code under test.

So, this PR is really about reducing mental complexity and helping to focus on the code we’re currently testing.

Thanks @fdesu Have no intention to discourage improvements, @andrross @cwperks do you folks have an opinion? For me personally the cognitive load has increased actually: instead of looking what dispatcher does in every single test, I have to go to the builder and figure out what does dispatcher do, separate class, far from the test case context.

[fdesu]

@reta Thanks! Sure, no worries at all, this is very valuable feedback! I was thinking sensible defaults + a lambda is simpler but that's just me so I'm absolutely fine if consensus is to avoid such change.

[cwperks]

@andrross @cwperks do you folks have an opinion?

Sorry just saw this now. I'll take a deeper look in the morning tomorrow and provide feedback. Thank you for these PRs @fdesu!

[cwperks]

I'm not opposed to the refactor, I see that it removes a bit of code duplication and has a net negative lines of code. I'm not sure if its possible, but could TestDispatcherBuilder move into test/framework to avoid duplicating the same file?

One other comment is that this needs to be rebased now that the changelog has been cleared after the 3.4 branch cut.

[fdesu]

@cwperks fyi I've rebased onto the latest main so the CI gets another rerun. A few flaky tests failed in the previous run:

> Task :server:internalClusterTest

Tests with failures:
 - org.opensearch.action.admin.cluster.filecache.PruneFileCacheIT.testPruneCacheWithRealData
 - org.opensearch.cluster.routing.allocation.decider.ShardsLimitAllocationDeciderIT.testCombinedClusterAndIndexSpecificShardLimits {p0={"opensearch.experimental.feature.writable_warm_index.enabled":"true"}}
 - org.opensearch.remotestore.RestoreShallowSnapshotV2IT.testContinuousIndexing {p0={"opensearch.experimental.feature.writable_warm_index.enabled":"false"}}
 - org.opensearch.remotestore.RestoreShallowSnapshotV2IT.classMethod

5594 tests completed, 4 failed, 62 skipped

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (1)

87-96: Add a descriptive failure message to the fail() call.

The lambda at line 88 calls fail() without a message. Adding a descriptive message would improve test failure diagnostics when debugging.

Apply this diff to add a descriptive message:

-        HttpServerTransport.Dispatcher dispatcher = dispatcherBuilderWithDefaults().withDispatchRequest(
-            (request, channel, threadContext) -> fail()
-        ).withDispatchBadRequest((channel, threadContext, cause) -> {
+        HttpServerTransport.Dispatcher dispatcher = dispatcherBuilderWithDefaults()
+            .withDispatchRequest((request, channel, threadContext) -> fail("Regular request should not be dispatched in bad request test"))
+            .withDispatchBadRequest((channel, threadContext, cause) -> {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 53f7df5 and 8f119fa.

📒 Files selected for processing (10)

• CHANGELOG.md (1 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (2 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (8 hunks)
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (12 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java (2 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (3 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java (9 hunks)
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (8 hunks)
• server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java (3 hunks)
• test/framework/src/main/java/org/opensearch/http/TestDispatcherBuilder.java (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (4)

• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4BadRequestTests.java
• CHANGELOG.md
• test/framework/src/main/java/org/opensearch/http/TestDispatcherBuilder.java
• server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

📚 Learning: 2025-12-12T18:40:01.303Z

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

Applied to files:

• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java

📚 Learning: 2025-12-12T13:31:41.214Z

Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.

Applied to files:

• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java

📚 Learning: 2025-12-13T20:16:08.819Z

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4Http3ServerTransport.java:101-123
Timestamp: 2025-12-13T20:16:08.819Z
Learning: In OpenSearch, only one HTTP transport implementation can be active and loaded at a time, so duplicate setting definitions (such as h3.max_stream_local_length, h3.max_stream_remote_length, and h3.max_streams) across different transport implementations like Netty4Http3ServerTransport and ReactorNetty4HttpServerTransport will not cause setting registration conflicts.

Applied to files:

• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java
• plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java
• modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java

🧬 Code graph analysis (5)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (1)

test/framework/src/main/java/org/opensearch/http/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (31-140)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (1)

test/framework/src/main/java/org/opensearch/http/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (31-140)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (1)

test/framework/src/main/java/org/opensearch/http/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (31-140)

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java (1)

test/framework/src/main/java/org/opensearch/http/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (31-140)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (1)

test/framework/src/main/java/org/opensearch/http/TestDispatcherBuilder.java (1)

• TestDispatcherBuilder (31-140)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)

• GitHub Check: gradle-check
• GitHub Check: detect-breaking-change
• GitHub Check: assemble (25, windows-latest)
• GitHub Check: assemble (25, ubuntu-24.04-arm)
• GitHub Check: assemble (25, ubuntu-latest)
• GitHub Check: assemble (21, ubuntu-latest)
• GitHub Check: assemble (21, ubuntu-24.04-arm)
• GitHub Check: assemble (21, windows-latest)
• GitHub Check: precommit (21, ubuntu-24.04-arm)
• GitHub Check: precommit (21, windows-latest)
• GitHub Check: precommit (21, windows-2025, true)
• GitHub Check: precommit (25, macos-15)
• GitHub Check: precommit (25, windows-latest)
• GitHub Check: precommit (21, macos-15)
• GitHub Check: precommit (25, ubuntu-24.04-arm)
• GitHub Check: precommit (21, macos-15-intel)
• GitHub Check: precommit (25, macos-15-intel)
• GitHub Check: precommit (21, ubuntu-latest)
• GitHub Check: Analyze (java)
• GitHub Check: precommit (25, ubuntu-latest)

🔇 Additional comments (22)

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4BadRequestTests.java (2)

63-63: LGTM!

The static import for dispatcherBuilderWithDefaults is appropriate and improves readability of the builder usage at line 87.

---

75-75: LGTM!

Removing the throws Exception declarations from setup() and shutdown() is appropriate since these methods don't actually throw checked exceptions.

Also applies to: 82-82

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/ssl/SecureNetty4HttpServerTransportTests.java (3)

146-146: Good cleanup: removing unnecessary throws clauses.

The throws Exception declarations have been correctly removed from setup() and shutdown() methods, as neither method throws checked exceptions.

Also applies to: 166-166

---

218-222: LGTM: dispatcher refactoring maintains test behavior.

The transition from inline anonymous Dispatcher implementations to the builder pattern with lambdas is correctly applied. The test logic is preserved and the code is more concise.

Also applies to: 316-326, 379-388

---

423-423: Appropriate use of default dispatcher.

Using dispatcherBuilderWithDefaults().build() is correct for these tests:

• testCorsRequest validates CORS handling that occurs before the dispatcher is invoked
• testReadTimeout tests timeout behavior where no complete request reaches the dispatcher

The default behavior (throwing AssertionError on unexpected invocations) provides appropriate test safety.

Also applies to: 478-478

modules/transport-netty4/src/test/java/org/opensearch/http/netty4/Netty4HttpServerTransportTests.java (6)

100-100: LGTM: Static import streamlines dispatcher creation.

The static import of dispatcherBuilderWithDefaults aligns with the PR's goal of reducing boilerplate in test setup.

---

117-117: LGTM: Removal of unnecessary exception declarations.

The throws Exception clauses were correctly removed from setup() and shutdown() since neither method throws checked exceptions in their implementations.

Also applies to: 125-125

---

174-178: LGTM: Dispatcher builder usage is clean and correct.

The builder-based dispatcher correctly preserves the original lambda behavior for dispatching requests and sending responses.

---

269-279: LGTM: Bad request handling correctly migrated to builder.

The withDispatchBadRequest configuration properly captures the exception handling logic that was previously in an anonymous dispatcher.

---

331-340: LGTM: Request handler logic preserved.

The URI validation and response logic for large compressed responses is correctly migrated to the builder's withDispatchRequest method.

---

391-391: LGTM: Default dispatcher usage is appropriate.

Using dispatcherBuilderWithDefaults().build() for tests that don't need custom dispatch logic is the right approach and simplifies setup.

Also applies to: 445-445

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java (4)

91-91: LGTM: Static import added consistently.

The static import follows the same pattern as other test files in this PR.

---

175-175: LGTM: Exception declaration removed from shutdown().

The throws Exception clause was correctly removed from shutdown() since it doesn't throw checked exceptions.

---

223-227: LGTM: Dispatcher builder usage is consistent.

All dispatcher creations correctly use the builder pattern with appropriate withDispatchRequest and withDispatchBadRequest configurations. The logic from the original anonymous dispatchers has been preserved.

Also applies to: 305-305, 351-355, 392-401, 451-451, 506-506

---

193-193: The removal of expectedStatus parameter is intentional but represents reduced test coverage.

The change is confirmed as intentional. Both secure and non-secure Reactor Netty tests include the same comment: "Reactor Netty 4 does not expose 100 CONTINUE response but instead just asks for content." This is a known limitation of Reactor Netty's protocol handling.

However, the non-secure ReactorNetty4HttpServerTransportTests validates three different expected status codes (CONTINUE, REQUEST_ENTITY_TOO_LARGE, EXPECTATION_FAILED), while the secure tests only validate the OK response. The non-secure tests include protocol-aware logic to adjust expectations based on HTTP version. Since secure transport typically uses HTTP/2+ protocols that don't expose 100-CONTINUE responses, the secure tests are limited to validating the happy path. This represents a deliberate but narrower test scope compared to the non-secure implementation.

⛔ Skipped due to learnings

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:01.303Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.

Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:41.214Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.

Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4Http3ServerTransport.java:101-123
Timestamp: 2025-12-13T20:16:08.819Z
Learning: In OpenSearch, only one HTTP transport implementation can be active and loaded at a time, so duplicate setting definitions (such as h3.max_stream_local_length, h3.max_stream_remote_length, and h3.max_streams) across different transport implementations like Netty4Http3ServerTransport and ReactorNetty4HttpServerTransport will not cause setting registration conflicts.

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportTests.java (3)

96-96: LGTM: Static import added consistently.

The import follows the standard pattern established across all test files in this PR.

---

111-111: LGTM: Unnecessary exception declarations removed.

Both setup() and shutdown() correctly have their throws Exception clauses removed since they don't throw checked exceptions.

Also applies to: 119-119

---

171-175: LGTM: Comprehensive dispatcher migration to builder pattern.

All dispatcher creations have been successfully migrated to use dispatcherBuilderWithDefaults(). The test logic is preserved:

• Expect header tests use custom request handlers
• Bad request and dispatch failure tests use default or minimal configuration
• Large response and connection closure tests use URI-specific logic

The refactoring reduces duplication and improves test readability.

Also applies to: 256-256, 300-304, 340-349, 400-409, 446-446, 500-500

plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransportStreamingTests.java (4)

56-56: LGTM: Static import added for builder utility.

The import of dispatcherBuilderWithDefaults enables the streaming dispatcher refactoring that follows.

---

64-64: LGTM: Lambda syntax simplified.

Removing unnecessary parentheses from the single-parameter lambda (str -> instead of (str) ->) is a minor style improvement.

---

77-77: LGTM: Exception declarations removed appropriately.

The throws Exception clauses were correctly removed from both setup() and shutdown() as these methods don't throw checked exceptions.

Also applies to: 85-85

---

179-217: LGTM: Streaming dispatcher successfully migrated to builder pattern.

The refactoring correctly preserves the complex streaming logic:

• withDispatchHandler returns a RestHandler that indicates streaming support
• withDispatchRequest implements the streaming response logic with proper channel writeability checks and reactive stream orchestration
• The URI validation and error handling remain intact

The builder approach improves clarity by separating handler routing (withDispatchHandler) from request processing (withDispatchRequest), making the streaming test setup more maintainable.

[github-actions]

❌ Gradle check result for 8f119fa: FAILURE

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?

[github-actions]

❌ Gradle check result for c813202: FAILURE

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?

[github-actions]

❌ Gradle check result for df4c4d6: FAILURE

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?

[github-actions]

❌ Gradle check result for df4c4d6: null

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?

[github-actions]

❌ Gradle check result for 6550fd9: FAILURE

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?

[github-actions]

✅ Gradle check result for 8d9a890: SUCCESS