opensearch-project
Cleanup HttpServerTransport.Dispatcher in Netty tests
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
- [x] 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
- Added a centralized test dispatcher builder used across Netty and Reactor Netty HTTP transport tests, reducing per-test boilerplate while preserving validations (headers, bad requests, streaming, timeouts, CORS, large responses); simplified setup/teardown signatures.
-
Chores
- Added changelog entry, enabled an auxiliary transport with a default port in config, updated a run configuration, and added a test-suite timeout annotation.
✏️ Tip: You can customize this high-level summary in your review settings.
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.javafor 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 Exceptionon 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[bot] avatar](https://avatars.githubusercontent.com/in/347564?v=4 "Published by a pub.dev verified publisher"){kind=small}
:x: Gradle check result for e980258469d8f0696f1557d3f63be84c9c37d050: 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[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 73.23%. Comparing base (930ae74) to head (8d9a890).
:warning: 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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
:x: Gradle check result for 7e8b64f54226b00ddd0548769caf441f8c6de4a8: 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?
:x: Gradle check result for e8963248c33b72b95fa9fe75e3f13f1ba4cc880d: 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 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.
:x: Gradle check result for 084ab2fd5796da9ae8ba2d52ae4e76c26d560d02: 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 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.
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.
@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.
@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.
@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.
@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!
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.
@cwperks thanks for the review! Yes, it is absolutely doable and thus a few other tests outside of netty modules could harness it which is even better. Originally I wasn't sure about adding things to the shared test framework but it looks like a perfect spot for such a thing. I'll take a quick look!
javadoc error now
/home/runner/work/OpenSearch/OpenSearch/test/framework/src/main/java/org/opensearch/http/TestDispatcherBuilder.java:51: error: self-closing element not allowed *
Task :test:framework:javadoc
:x: Gradle check result for 09e0db60a1a0994383b7ac4a8261ffe42c8ddf53: 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?
ugh... looks like CI runners are under pressure right now:
[2025-12-16 16:37:17] [autobuild] Picked up JAVA_TOOL_OPTIONS: -Dhttp.keepAlive=false -Dmaven.wagon.http.pool=false
[2025-12-16 16:37:18] [autobuild] Downloading https://services.gradle.org/distributions/gradle-9.2.0-all.zip
[2025-12-16 16:37:30] [autobuild] Exception in thread "main" java.io.IOException: Downloading from https://services.gradle.org/distributions/gradle-9.2.0-all.zip failed: timeout (10000ms)
[2025-12-16 16:37:30] [autobuild] at org.gradle.wrapper.Install.forceFetch(SourceFile:4)
[2025-12-16 16:37:30] [autobuild] at org.gradle.wrapper.Install$1.call(SourceFile:8)
[2025-12-16 16:37:30] [autobuild] at org.gradle.wrapper.GradleWrapperMain.main(SourceFile:67)
[2025-12-16 16:37:30] [autobuild] Caused by: java.net.SocketTimeoutException: Read timed out
first it failed to fetch jdk 25 and now the gradle distro.
:x: Gradle check result for 53f7df5d66ace1a64c5f4755c467c63f25a1e68e: 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?
@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
:x: Gradle check result for 8f119fab5afaf988417f7e14fd3921dbdf39f288: 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?
:x: Gradle check result for c8132022d8411a462ac1fde174f0d184d08b9a90: 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?
:x: Gradle check result for df4c4d6af44103221e4573ac4fa9e5e297d436c3: 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?
:x: Gradle check result for df4c4d6af44103221e4573ac4fa9e5e297d436c3: 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?
:x: Gradle check result for 6550fd9476c45fc14a70342a68c4a794040386f0: 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?