Refactor ShardStats, WarmerStats and IndexingPressureStats with Builder pattern by pado0 · Pull Request #19966 · opensearch-project/OpenSearch

pado0 · 2025-11-12T04:25:24Z

Description

This PR refactors the ShardStats, WarmerStats and IndexingPressureStats class to use the Builder pattern instead of relying on multiple constructors.

By adopting the Builder pattern, it becomes easier to evolve the stats API, add new metrics, and maintain backward compatibility without forcing disruptive constructor changes.

Based on the related issue:

Added a Builder and deprecated the existing constructors.
Replaced usages of constructors in code and tests with the new Builder.

There are multiple stats-related classes that need similar refactoring, and we are addressing them in priority order. This PR covers ShardStats, WarmerStats and IndexingPressureStats as part of that effort.

Related Issues

Related to #19225

Check List

Functionality includes testing.

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

Refactor
- Switched multiple internal stats classes (shard, warmer, indexing pressure) to builder-based initialization, preserving behavior and keeping deprecated legacy constructors for compatibility.
Tests
- Updated tests to use the new builder-based patterns.
Documentation
- Added a changelog entry noting the refactor and deprecations.

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

github-actions · 2025-11-12T05:16:42Z

❌ Gradle check result for 3e77dca: 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 · 2025-11-12T06:52:41Z

❌ Gradle check result for 3e77dca: 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 · 2025-11-12T07:07:22Z

❌ Gradle check result for 3e77dca: 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

♻️ Duplicate comments (1)

server/src/main/java/org/opensearch/action/admin/indices/stats/ShardStats.java (1)
117-121: Consider improving deprecation guidance.

The past review comment suggested making the migration path more explicit by referencing ShardStats.Builder instead of just Builder, or providing a brief usage hint like ShardStats.builder().<setters>().build().

Apply this diff to provide clearer migration guidance:
 /**
  * This constructor will be deprecated starting in version 3.4.0.
- * Use {@link Builder} instead.
+ * Use {@link ShardStats.Builder} to construct instances instead.
+ * Example: {@code new ShardStats.Builder().shardRouting(...).shardPath(...).commonStats(...).build()}
  */

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f8ee10f and 9af35fe.

📒 Files selected for processing (1)

server/src/main/java/org/opensearch/action/admin/indices/stats/ShardStats.java (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

server/src/main/java/org/opensearch/action/admin/indices/stats/ShardStats.java (1)

server/src/main/java/org/opensearch/index/stats/IndexingPressureStats.java (1)

Builder (141-224)

⏰ 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: check-files
GitHub Check: detect-breaking-change
GitHub Check: precommit (25, macos-15-intel)
GitHub Check: precommit (25, ubuntu-24.04-arm)
GitHub Check: precommit (21, ubuntu-24.04-arm)
GitHub Check: precommit (25, macos-15)
GitHub Check: precommit (21, windows-2025, true)
GitHub Check: precommit (21, macos-15)
GitHub Check: precommit (25, windows-latest)
GitHub Check: precommit (25, ubuntu-latest)
GitHub Check: precommit (21, ubuntu-latest)
GitHub Check: precommit (21, macos-15-intel)
GitHub Check: precommit (21, windows-latest)
GitHub Check: assemble (25, ubuntu-latest)
GitHub Check: assemble (25, ubuntu-24.04-arm)
GitHub Check: assemble (25, windows-latest)
GitHub Check: assemble (21, windows-latest)
GitHub Check: assemble (21, ubuntu-24.04-arm)
GitHub Check: assemble (21, ubuntu-latest)
GitHub Check: Analyze (java)

server/src/main/java/org/opensearch/action/admin/indices/stats/ShardStats.java

github-actions · 2025-11-27T07:27:10Z

❌ Gradle check result for 9af35fe: 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 · 2025-11-27T13:18:56Z

❌ Gradle check result for 9af35fe: 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 · 2025-11-27T21:12:13Z

❌ Gradle check result for 9af35fe: 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 · 2025-11-27T22:28:59Z

✅ Gradle check result for 9af35fe: SUCCESS

codecov · 2025-11-27T22:29:37Z

Codecov Report

❌ Patch coverage is 94.40559% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.18%. Comparing base (97d3864) to head (022b88e).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
...in/java/org/opensearch/indices/IndicesService.java	0.00%	8 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #19966      +/-   ##
============================================
- Coverage     73.33%   73.18%   -0.16%     
+ Complexity    71679    71619      -60     
============================================
  Files          5790     5786       -4     
  Lines        327549   327754     +205     
  Branches      47181    47206      +25     
============================================
- Hits         240217   239871     -346     
- Misses        68080    68657     +577     
+ Partials      19252    19226      -26

☔ 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.

pado0 · 2025-11-28T04:59:11Z

❌ Gradle check result for 9af35fe: 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?

It succeeded on my local.

Signed-off-by: Jean Kim <bgshhd95@gmail.com>

coderabbitai

Actionable comments posted: 0

♻️ Duplicate comments (1)

server/src/main/java/org/opensearch/action/admin/indices/stats/ShardStats.java (1)
91-101: Null validation still missing - risk of NPE at runtime.

The private constructor dereferences builder.shardPath (lines 93-95) and other builder fields without null checks. If build() is called without setting required fields (shardPath, shardRouting, commonStats), a NullPointerException will be thrown.

This issue was previously flagged. Please add validation in the Builder.build() method (lines 234-236) before constructing ShardStats:
 public ShardStats build() {
+    if (shardRouting == null) {
+        throw new IllegalStateException("shardRouting must be set");
+    }
+    if (shardPath == null) {
+        throw new IllegalStateException("shardPath must be set");
+    }
+    if (commonStats == null) {
+        throw new IllegalStateException("commonStats must be set");
+    }
     return new ShardStats(this);
 }

🧹 Nitpick comments (2)

server/src/main/java/org/opensearch/index/stats/IndexingPressureStats.java (1)

67-85: IndexingPressureStats.Builder implementation and deprecation strategy look sound

The private constructor correctly copies every field from Builder, and the builder exposes setters for all existing stats plus memoryLimit, so behavior matches the old all‑args constructor.

Serialization and deserialization paths (StreamInput ctor and writeTo) are unchanged, so wire compatibility is preserved while internal construction moves to the builder.

Very minor nit: the Javadoc on the private constructor calling it “the sole entry point for creating a new IndexingPressureStats object” is no longer strictly true with the public (deprecated) ctor and StreamInput ctor still present. If you want to avoid confusion for future readers, consider softening that wording (e.g., “primary entry point used by the Builder”).

Also applies to: 104-108, 137-225
server/src/main/java/org/opensearch/action/admin/indices/stats/ShardStats.java (1)
117-121: Deprecation javadoc could be more explicit about migration path.

The javadoc references {@link Builder} which is an improvement, but could provide a clearer migration example for developers.

Consider adding a brief migration hint:
 /**
  * This constructor will be deprecated starting in version 3.4.0.
- * Use {@link Builder} instead.
+ * Use {@link Builder} instead. Example: 
+ * {@code new ShardStats.Builder().shardRouting(...).shardPath(...).commonStats(...).build()}
  */

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9af35fe and 022b88e.

📒 Files selected for processing (18)

CHANGELOG.md (2 hunks)
server/src/main/java/org/opensearch/action/admin/cluster/stats/TransportClusterStatsAction.java (2 hunks)
server/src/main/java/org/opensearch/action/admin/indices/stats/ShardStats.java (3 hunks)
server/src/main/java/org/opensearch/action/admin/indices/stats/TransportIndicesStatsAction.java (1 hunks)
server/src/main/java/org/opensearch/index/IndexingPressure.java (1 hunks)
server/src/main/java/org/opensearch/index/stats/IndexingPressureStats.java (3 hunks)
server/src/main/java/org/opensearch/index/warmer/ShardIndexWarmerService.java (1 hunks)
server/src/main/java/org/opensearch/index/warmer/WarmerStats.java (2 hunks)
server/src/main/java/org/opensearch/indices/IndicesService.java (1 hunks)
server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java (2 hunks)
server/src/test/java/org/opensearch/action/admin/cluster/stats/ClusterStatsNodesTests.java (1 hunks)
server/src/test/java/org/opensearch/action/admin/cluster/stats/ClusterStatsResponseTests.java (1 hunks)
server/src/test/java/org/opensearch/action/admin/indices/rollover/TransportRolloverActionTests.java (1 hunks)
server/src/test/java/org/opensearch/action/admin/indices/shards/CatShardsResponseTests.java (1 hunks)
server/src/test/java/org/opensearch/action/admin/indices/stats/IndicesStatsResponseTests.java (1 hunks)
server/src/test/java/org/opensearch/cluster/DiskUsageTests.java (1 hunks)
server/src/test/java/org/opensearch/index/shard/IndexShardTests.java (2 hunks)
server/src/test/java/org/opensearch/rest/action/cat/RestShardsActionTests.java (1 hunks)

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

server/src/main/java/org/opensearch/index/warmer/ShardIndexWarmerService.java
server/src/test/java/org/opensearch/action/admin/cluster/stats/ClusterStatsNodesTests.java
server/src/main/java/org/opensearch/index/warmer/WarmerStats.java
server/src/main/java/org/opensearch/indices/IndicesService.java
server/src/test/java/org/opensearch/index/shard/IndexShardTests.java
server/src/test/java/org/opensearch/action/admin/indices/stats/IndicesStatsResponseTests.java
CHANGELOG.md

🧰 Additional context used

🧬 Code graph analysis (2)

server/src/main/java/org/opensearch/index/stats/IndexingPressureStats.java (1)

server/src/main/java/org/opensearch/action/admin/indices/stats/ShardStats.java (1)

Builder (184-237)

server/src/main/java/org/opensearch/action/admin/indices/stats/ShardStats.java (1)

server/src/main/java/org/opensearch/index/stats/IndexingPressureStats.java (1)

Builder (141-224)

⏰ 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: assemble (25, windows-latest)
GitHub Check: precommit (25, macos-15)
GitHub Check: precommit (25, windows-latest)
GitHub Check: precommit (25, macos-15-intel)
GitHub Check: precommit (25, ubuntu-24.04-arm)
GitHub Check: precommit (21, windows-2025, true)
GitHub Check: precommit (21, ubuntu-24.04-arm)
GitHub Check: precommit (25, ubuntu-latest)
GitHub Check: precommit (21, windows-latest)
GitHub Check: assemble (21, windows-latest)
GitHub Check: precommit (21, macos-15-intel)
GitHub Check: assemble (25, ubuntu-latest)
GitHub Check: precommit (21, macos-15)
GitHub Check: assemble (25, ubuntu-24.04-arm)
GitHub Check: assemble (21, ubuntu-24.04-arm)
GitHub Check: precommit (21, ubuntu-latest)
GitHub Check: assemble (21, ubuntu-latest)
GitHub Check: Analyze (java)
GitHub Check: detect-breaking-change

🔇 Additional comments (9)

server/src/main/java/org/opensearch/index/IndexingPressure.java (1)

220-233: IndexingPressureStats builder wiring looks correct

The builder invocation sets all the same fields that were previously passed to the constructor, preserving semantics including the memoryLimit value from primaryAndCoordinatingLimits. Looks good.

server/src/test/java/org/opensearch/action/admin/indices/shards/CatShardsResponseTests.java (1)

155-164: ShardStats test construction migrated cleanly to Builder

The builder call mirrors the previous constructor usage (routing, path, CommonStats, and null optional stats). No behavior change; tests remain clear and aligned with the new API.

server/src/test/java/org/opensearch/cluster/DiskUsageTests.java (1)

138-153: ShardStats.Builder usage in disk usage test is equivalent to prior ctor

Both shard entries correctly use shardRouting, ShardPath, CommonStats, and explicit nulls for optional stats, matching the old constructor contract.

server/src/main/java/org/opensearch/action/admin/indices/stats/TransportIndicesStatsAction.java (1)

158-165: ShardStats.Builder wiring in shardOperation() matches previous constructor

The builder sets routingEntry, shardPath, commonStats, and all optional stats (commitStats, seqNoStats, retentionLeaseStats, pollingIngestStats) exactly as before, so behavior is preserved while adopting the new pattern.

server/src/main/java/org/opensearch/action/admin/cluster/stats/TransportClusterStatsAction.java (1)

228-235: Cluster stats shard collection now correctly uses ShardStats.Builder

The builder call in shardsStats.add(...) sets routing, shard path, CommonStats, and the four stats objects identically to the old constructor. The small Javadoc formatting tweak on isMetricRequired is harmless.

Also applies to: 259-259

server/src/test/java/org/opensearch/action/admin/indices/rollover/TransportRolloverActionTests.java (1)

505-513: Rollover tests use ShardStats.Builder consistently with previous ctor

The builder setup (routing, ShardPath, rich CommonStats, null optional stats) is a direct translation of the old constructor-based call; good alignment with the new API.

server/src/test/java/org/opensearch/action/admin/cluster/stats/ClusterStatsResponseTests.java (1)

277-284: Cluster stats response tests correctly adopt ShardStats.Builder

createShardStats now uses the builder to set routing, shard path, and CommonStats, with explicit nulls for optional stats, preserving the prior test behavior.

server/src/test/java/org/opensearch/rest/action/cat/RestShardsActionTests.java (1)

90-97: LGTM - Builder pattern migration complete.

The ShardStats construction correctly uses the new Builder pattern with all required fields (shardRouting, shardPath, commonStats) and explicitly sets optional stats to null.

server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java (1)

1421-1428: LGTM - Consistent Builder pattern migration.

Both ShardStats construction sites correctly use the Builder pattern with all required fields and explicit null values for optional stats. The migration is consistent across the test suite.

Also applies to: 1472-1479

github-actions · 2025-11-28T06:08:16Z

❌ Gradle check result for 022b88e: 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 · 2025-11-28T07:26:56Z

✅ Gradle check result for 022b88e: SUCCESS

sandeshkr419 · 2025-11-28T07:32:32Z

Thanks @pado0 for being patient on the last one with CI failure noise. Thanks for working through all these refactoring PRs.
Happy Thanksgiving! :D

pado0 closed this Nov 12, 2025

pado0 reopened this Nov 12, 2025

This was referenced Nov 12, 2025

[AUTOCUT] Gradle Check Flaky Test Report for FullClusterRestartIT #15813

Open

[AUTOCUT] Gradle Check Flaky Test Report for IndexingIT #14302

Open

[AUTOCUT] Gradle Check Flaky Test Report for InternalDistributionBwcSetupPluginFuncTest #19808

Open

pado0 closed this Nov 12, 2025

pado0 reopened this Nov 12, 2025

This was referenced Nov 12, 2025

[AUTOCUT] Gradle Check Flaky Test Report for RemoteSplitIndexIT #14296

Open

[AUTOCUT] Gradle Check Flaky Test Report for SharedClusterSnapshotRestoreIT #15845

Open

[AUTOCUT] Gradle Check Flaky Test Report for IndexStatsIT #15836

Open

pado0 closed this Nov 13, 2025

pado0 reopened this Nov 13, 2025

pado0 marked this pull request as ready for review November 13, 2025 06:27

pado0 requested review from Bukhtawar, CEHENKLE, anasalkouz, andrross, ashking94, cwperks, dbwiddis, gbbafna, jed326, kotwanikunal, mch2, msfroh, owaiskazi19 and reta as code owners November 13, 2025 06:27

coderabbitai bot reviewed Nov 27, 2025

View reviewed changes

server/src/main/java/org/opensearch/action/admin/indices/stats/ShardStats.java Show resolved Hide resolved

opensearch-ci-bot mentioned this pull request Nov 27, 2025

[AUTOCUT] Gradle Check Flaky Test Report for Gradle Test Run :plugins:telemetry-otel:test #19805

Open

opensearch-ci-bot mentioned this pull request Nov 27, 2025

[AUTOCUT] Gradle Check Flaky Test Report for IndexActionIT #16576

Open

opensearch-ci-bot mentioned this pull request Nov 27, 2025

[AUTOCUT] Gradle Check Flaky Test Report for Netty4HttpRequestSizeLimitIT #18875

Open

pado0 added 6 commits November 27, 2025 20:59

Add Builder in ShardStats and refactor usage

a84e420

Signed-off-by: Jean Kim <bgshhd95@gmail.com>

Add Builder in WarmerStats and refactor usage

068f005

Signed-off-by: Jean Kim <bgshhd95@gmail.com>

Add Builder in IndexingPressure and refactor usage

7d7a375

Signed-off-by: Jean Kim <bgshhd95@gmail.com>

Add Change Log and remove unused comment

960a8d6

Signed-off-by: Jean Kim <bgshhd95@gmail.com>

Fix typo bug - seqNoStat -> seqNoStats

0b85f6c

Signed-off-by: Jean Kim <bgshhd95@gmail.com>

Update Comment

022b88e

Signed-off-by: Jean Kim <bgshhd95@gmail.com>

coderabbitai bot reviewed Nov 28, 2025

View reviewed changes

This was referenced Nov 28, 2025

[AUTOCUT] Gradle Check Flaky Test Report for RemoteRestoreSnapshotIT #14324

Open

[AUTOCUT] Gradle Check Flaky Test Report for ShardIndexingPressureIT #15830

Closed

sandeshkr419 mentioned this pull request Dec 7, 2025

[Refactor] Use Builder pattern instead of constructors for Stats API classes #19225

Closed

coderabbitai bot mentioned this pull request Dec 13, 2025

Cleanup HttpServerTransport.Dispatcher in Netty tests #20160

Merged

1 task

BrewTestBot mentioned this pull request Dec 17, 2025

opensearch 3.4.0 Homebrew/homebrew-core#258986

Merged

tkykenmt mentioned this pull request Jan 11, 2026

[feature] Stats Builder Pattern Refactoring tkykenmt/opensearch-feature-explorer#1700

Closed

3 tasks

opensearch-ci-bot mentioned this pull request Mar 27, 2026

[AUTOCUT] Gradle Check Flaky Test Report for RemoteShrinkIndexIT #19846

Open

Conversation

pado0 commented Nov 12, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Summary by CodeRabbit

Uh oh!

github-actions bot commented Nov 12, 2025

Uh oh!

github-actions bot commented Nov 12, 2025

Uh oh!

github-actions bot commented Nov 12, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Nov 27, 2025

Uh oh!

github-actions bot commented Nov 27, 2025

Uh oh!

github-actions bot commented Nov 27, 2025

Uh oh!

github-actions bot commented Nov 27, 2025

Uh oh!

codecov bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pado0 commented Nov 28, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 28, 2025

Uh oh!

github-actions bot commented Nov 28, 2025

Uh oh!

sandeshkr419 commented Nov 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pado0 commented Nov 12, 2025 •

edited by coderabbitai bot

Loading

codecov bot commented Nov 27, 2025 •

edited

Loading