Remove legacy client benchmark and noop API plugin modules - #21839
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Removes the legacy client benchmarking tooling (the client/benchmark app and the client-benchmark-noop-api-plugin) from the repository’s Gradle build and dependency automation configuration.
Changes:
- Removes
client:benchmarkandclient:client-benchmark-noop-api-pluginfrom the Gradle multi-project build. - Deletes the benchmark application sources/resources and the noop benchmark plugin sources/build files.
- Updates Dependabot configuration to stop tracking the removed Gradle subprojects.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle | Removes the benchmark and noop benchmark plugin projects from the included Gradle projects list. |
| gradle/missing-javadoc.gradle | Stops applying missing-javadoc configuration to the removed benchmark-related projects. |
| .github/dependabot.yml | Removes Dependabot update entries for the deleted Gradle subprojects. |
| client/client-benchmark-noop-api-plugin/build.gradle | Deletes the Gradle build definition for the noop benchmark plugin subproject. |
| client/client-benchmark-noop-api-plugin/README.md | Removes documentation for the deleted noop benchmark plugin. |
| client/client-benchmark-noop-api-plugin/src/main/java/org/opensearch/plugin/noop/NoopPlugin.java | Deletes the noop plugin entrypoint (plugin removed). |
| client/client-benchmark-noop-api-plugin/src/main/java/org/opensearch/plugin/noop/action/bulk/NoopBulkAction.java | Deletes noop bulk action type (plugin removed). |
| client/client-benchmark-noop-api-plugin/src/main/java/org/opensearch/plugin/noop/action/bulk/RestNoopBulkAction.java | Deletes noop bulk REST handler (plugin removed). |
| client/client-benchmark-noop-api-plugin/src/main/java/org/opensearch/plugin/noop/action/bulk/TransportNoopBulkAction.java | Deletes noop bulk transport action (plugin removed). |
| client/client-benchmark-noop-api-plugin/src/main/java/org/opensearch/plugin/noop/action/search/NoopSearchAction.java | Deletes noop search action type (plugin removed). |
| client/client-benchmark-noop-api-plugin/src/main/java/org/opensearch/plugin/noop/action/search/RestNoopSearchAction.java | Deletes noop search REST handler (plugin removed). |
| client/client-benchmark-noop-api-plugin/src/main/java/org/opensearch/plugin/noop/action/search/TransportNoopSearchAction.java | Deletes noop search transport action (plugin removed). |
| client/benchmark/build.gradle | Deletes the Gradle build definition for the benchmark application subproject. |
| client/benchmark/README.md | Removes documentation for the deleted benchmark application. |
| client/benchmark/src/main/resources/log4j2.properties | Deletes benchmark logging configuration (benchmark app removed). |
| client/benchmark/src/main/java/org/opensearch/client/benchmark/AbstractBenchmark.java | Deletes benchmark harness base class (benchmark app removed). |
| client/benchmark/src/main/java/org/opensearch/client/benchmark/BenchmarkMain.java | Deletes benchmark CLI entrypoint (benchmark app removed). |
| client/benchmark/src/main/java/org/opensearch/client/benchmark/BenchmarkRunner.java | Deletes benchmark runner/formatter (benchmark app removed). |
| client/benchmark/src/main/java/org/opensearch/client/benchmark/BenchmarkTask.java | Deletes benchmark task interface (benchmark app removed). |
| client/benchmark/src/main/java/org/opensearch/client/benchmark/metrics/Metrics.java | Deletes benchmark metrics DTO (benchmark app removed). |
| client/benchmark/src/main/java/org/opensearch/client/benchmark/metrics/MetricsCalculator.java | Deletes benchmark metrics calculations (benchmark app removed). |
| client/benchmark/src/main/java/org/opensearch/client/benchmark/metrics/Sample.java | Deletes benchmark sample model (benchmark app removed). |
| client/benchmark/src/main/java/org/opensearch/client/benchmark/metrics/SampleRecorder.java | Deletes benchmark sample recorder (benchmark app removed). |
| client/benchmark/src/main/java/org/opensearch/client/benchmark/ops/bulk/BulkBenchmarkTask.java | Deletes bulk benchmark task implementation (benchmark app removed). |
| client/benchmark/src/main/java/org/opensearch/client/benchmark/ops/bulk/BulkRequestExecutor.java | Deletes bulk request executor interface (benchmark app removed). |
| client/benchmark/src/main/java/org/opensearch/client/benchmark/ops/search/SearchBenchmarkTask.java | Deletes search benchmark task implementation (benchmark app removed). |
| client/benchmark/src/main/java/org/opensearch/client/benchmark/ops/search/SearchRequestExecutor.java | Deletes search request executor interface (benchmark app removed). |
| client/benchmark/src/main/java/org/opensearch/client/benchmark/rest/RestClientBenchmark.java | Deletes REST client benchmark implementation (benchmark app removed). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR Reviewer Guide 🔍(Review updated until commit d5051b5)Here are some key observations to aid the review process:
|
|
Persistent review updated to latest commit 46a7c35 |
|
❌ Gradle check result for 46a7c35: 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? |
|
Gradle check build 78356 failed. After investigation, there are no remaining references to the deleted modules ( This failure appears to be a flaky test unrelated to the PR changes. Retriggering the gradle-check now. |
|
Persistent review updated to latest commit c0a430d |
|
❌ Gradle check result for c0a430d: 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? |
|
Build 78421 also failed. After further investigation, this is not caused by the PR changes. Evidence of systemic CI flakiness:
Retriggering gradle-check again. |
|
Persistent review updated to latest commit 7f6e74b |
|
❌ Gradle check result for 7f6e74b: 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? |
|
The latest CI run (26497576780) is failing with a different error — GitHub Actions receives This is a CI infrastructure issue (GitHub Actions ↔ Jenkins authentication), not caused by the PR changes. Could a maintainer please manually trigger the gradle-check, or confirm when the CI infrastructure is back up? |
|
Persistent review updated to latest commit a1a03ed |
|
❌ Gradle check result for a1a03ed: 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? |
|
CI Infrastructure Issue — Jenkins trigger token expired After inspecting the GitHub Actions logs for runs 26497576780 and 26512183477, the root cause is clear: The This is not caused by the PR changes. The same failure is occurring on at least PR #21845 as well (failed at 10:49 UTC today in under 6 minutes), indicating the Jenkins Stopping retrigger attempts until the CI infrastructure is restored. @cwperks @peternied — could you help rotate the Jenkins trigger token or confirm when CI will be back up? |
|
Persistent review updated to latest commit 4442941 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21839 +/- ##
============================================
+ Coverage 71.48% 71.59% +0.11%
+ Complexity 77022 77005 -17
============================================
Files 6156 6139 -17
Lines 358482 358059 -423
Branches 52246 52218 -28
============================================
+ Hits 256255 256362 +107
+ Misses 81810 81302 -508
+ Partials 20417 20395 -22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Persistent review updated to latest commit e25ba6d |
|
❌ Gradle check result for e25ba6d: 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? |
|
Persistent review updated to latest commit 4168873 |
|
@gingeekrishna @cwperks isn't a bit premature to remove these benchmarks? may be we could make it a part of the published benchmarks instead? The code seems to be intact and I could update the instructions to point to the OpenSearch specific datasets and change the commands appropriately, wdyt? Thank you, |
|
@reta agreed, I opened up the issue for discussion without any intention of proceeding with the removal without confirmation. I've been in a "spring cleaning" mode and trying to identify stuff in the core that may no longer be used anywhere. I didn't know about this one and opened an issue in case anyone could comment on its use. |
The client/benchmark harness and client/client-benchmark-noop-api-plugin have no references from the maintained opensearch-benchmark project and are not executed by any CI workflow. Keeping them incurs ongoing build and dependency maintenance cost with no active usage. Remove both modules and clean up references in: - settings.gradle - gradle/missing-javadoc.gradle - .github/dependabot.yml Resolves opensearch-project#21708 Signed-off-by: Radhakrishnan Pachyappan <gingeekrishna@gmail.com>
fa18541 to
8bb0db5
Compare
|
Rebased onto latest Addressing outstanding concerns from @reta and @cwperks: @reta — you raised a concern on June 1 about preserving the client benchmark for use in published benchmarks. Since then, you committed Deprecate REST client (#22116) (June 12), adding @cwperks — you noted on June 2 that removal of these modules would typically be deferred to the next major release. Given the REST client deprecation in 3.x (#22116), is the same guidance still applicable, or does the REST client deprecation make removal appropriate in 3.x as well? Happy to close this PR if the decision is to keep the benchmarks through 3.x — just want to make sure I have the right picture before proceeding or withdrawing. |
|
Persistent review updated to latest commit 8bb0db5 |
|
❌ Gradle check result for 8bb0db5: 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? |
|
Persistent review updated to latest commit 26ff56f |
Thanks @gingeekrishna , my apologies for delay, I think the general direction to get rid of RestClient is now cleared out, the question that remains (for me at least), how useful this harness is, and if it is, should we repurpose for internal client instead? I will spend some time over the weekend on that, will share my findings shortly, thank you |
|
Thanks @reta, no worries at all on the delay — appreciate you taking the time to dig into this. A few data points that might help your investigation: The harness is wired entirely to If the goal is a benchmark for the internal transport client, I'd expect the OpenSearch Benchmark project is a better home for that work — it already has infrastructure for running coordinated load tests against live clusters with proper result publishing. That said, I'm happy to either:
Looking forward to hearing what you find — take your time! |
|
Persistent review updated to latest commit 7140b29 |
|
❌ Gradle check result for 7140b29: 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? |
Just spent some (more) time on it, the harness does look to be in healthy state and would agree, there is not much value in keeping it but not using at all, let's get rid of it, thanks @gingeekrishna , @cwperks wdyt? |
|
Persistent review updated to latest commit d116546 |
|
❕ Gradle check result for d116546: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
|
Persistent review updated to latest commit d5051b5 |
reta
added
enhancement
@reta agreed, sorry I missed this comment from a couple months ago. |
Description
Removes two legacy modules that have no active usage and are not referenced by the maintained opensearch-benchmark project:
client/benchmark— a REST client benchmark harness (RestClientBenchmark, bulk/search tasks, metrics)client/client-benchmark-noop-api-plugin— a noop OpenSearch plugin providing synthetic/_noop_bulkand/_noop_searchendpoints solely to support the above harnessInvestigation findings:
_noop_bulk,_noop_search,RestClientBenchmark, orclient-benchmark-noop-api-pluginexist inopensearch-project/opensearch-benchmarksettings.gradle,gradle/missing-javadoc.gradle, and.github/dependabot.yml— all removed in this PRFiles removed: 26 source/config files (2001 lines)
Build file changes:
settings.gradle— removed two module includesgradle/missing-javadoc.gradle— removed two project references.github/dependabot.yml— removed two dependabot entriesRelated Issues
Resolves #21708
Check List
/_noop_bulk,/_noop_search) are not part of the supported OpenSearch API surface.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.