Refactor ShardStats, WarmerStats and IndexingPressureStats with Builder pattern #19966
Conversation
|
❌ 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? |
|
❌ 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? |
|
❌ 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? |
pado0
requested review from
Bukhtawar,
CEHENKLE,
anasalkouz,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19 and
reta
as code owners
|
❌ Gradle check result for 2d2b317: 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? |
WalkthroughRefactored ShardStats, WarmerStats, and IndexingPressureStats to use Builder patterns; deprecated existing multi-arg constructors. Production and test code updated to instantiate these stats via their Builders; no behavioral or control-flow changes beyond object construction. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.0)server/src/test/java/org/opensearch/index/shard/IndexShardTests.javaTip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
@sandeshkr419 I'm really sorry I initially thought it was just a flaky test, Here’s the command I used: I’ve fixed it now. I’ll be more careful from next time. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
server/src/main/java/org/opensearch/index/stats/IndexingPressureStats.java (1)
104-108: Clarify the deprecation Javadoc wording.The Javadoc states "will be deprecated starting in version 3.4.0", but the constructor is already annotated with
@Deprecated. Consider updating the wording to accurately reflect the current state:/** - * This constructor will be deprecated starting in version 3.4.0. + * @deprecated As of version 3.4.0, use {@link Builder} instead. * Use {@link Builder} instead. */ @Deprecatedserver/src/main/java/org/opensearch/action/admin/indices/stats/ShardStats.java (1)
86-101: Tighten Builder null‑safety and fix constructor doc wordingNice move to a Builder here; it matches the pattern used in other stats classes. A couple of follow‑ups would make it safer and clearer:
- Required fields vs. NPEs at runtime
ShardStats(Builder)dereferencesbuilder.shardPath(and downstream uses assumeshardRoutingandcommonStatsare non‑null), but the Builder allows all fields to staynull. A caller can currently donew ShardStats.Builder().build()and hit aNullPointerException.Previously, the multi‑arg ctor enforced at compile time that
ShardRouting,ShardPath, andCommonStatswere provided. With the Builder we’ve lost that guard.Consider enforcing these invariants in
build()(or the private ctor), e.g.:@@ - public ShardStats build() { - return new ShardStats(this); - } + public ShardStats build() { + // Required fields – previously enforced by the public constructor + Objects.requireNonNull(shardRouting, "shardRouting must not be null"); + Objects.requireNonNull(shardPath, "shardPath must not be null"); + Objects.requireNonNull(commonStats, "commonStats must not be null"); + return new ShardStats(this); + }and add the corresponding import near the other imports:
import java.util.Objects;Optional stats (
commitStats,seqNoStats,retentionLeaseStats,pollingIngestStats) can stay nullable.
- Constructor javadoc accuracy
The comment above the private ctor:
/** * Private constructor that takes a builder. * This is the sole entry point for creating a new ShardStats object. */isn’t strictly accurate while the stream ctor and the deprecated ctor remain. Suggest rephrasing to focus on “new API entry point”, e.g.:
- /** - * Private constructor that takes a builder. - * This is the sole entry point for creating a new ShardStats object. - * @param builder The builder instance containing all the values. - */ + /** + * Private constructor used by the {@link ShardStats.Builder}. + * This is the entry point for programmatic construction of a new ShardStats instance. + * @param builder The builder instance containing all the values. + */Also applies to: 180-237
server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java (1)
1421-1428: Builder usage forShardStatsin tests looks good (optional cleanup)The migration to:
ShardStats shardStats = new ShardStats.Builder().shardRouting(shardRouting) .shardPath(new ShardPath(false, path, path, shardRouting.shardId())) .commonStats(...) .commitStats(null) .seqNoStats(null) .retentionLeaseStats(null) .pollingIngestStats(null) .build();is correct and mirrors the old ctor parameter order/semantics.
If you’d like to trim noise, the explicit
*.xxx(null)calls are redundant since those fields default tonullin the Builder; you can omit them entirely for brevity:ShardStats shardStats = new ShardStats.Builder().shardRouting(shardRouting) .shardPath(new ShardPath(false, path, path, shardRouting.shardId())) .commonStats(createRandomCommonStats()) // or commonStats .build();Also applies to: 1472-1479
server/src/test/java/org/opensearch/index/shard/IndexShardTests.java (1)
1838-1845: Consider a small helper to reduce ShardStats builder duplicationThe
ShardStats.Builderchain here is identical to the one intestShardStats. A tiny test helper likeprivate ShardStats buildShardStats(IndexShard shard)would remove duplication and keep future builder field additions in one place. Optional, but could ease future evolution of the stats API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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)
🧰 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). (21)
- GitHub Check: gradle-check
- GitHub Check: detect-breaking-change
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, macos-15)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: Analyze (java)
🔇 Additional comments (17)
CHANGELOG.md (1)
70-70: LGTM! Changelog entries are well-structured.The changelog correctly documents this refactoring in both the "Changed" and "Deprecated" sections, maintaining consistency with similar builder pattern migrations in this release.
Also applies to: 139-139
server/src/main/java/org/opensearch/index/warmer/WarmerStats.java (1)
63-72: LGTM! Builder pattern correctly implemented.The Builder implementation follows best practices:
- Private constructor accepting Builder ensures controlled instantiation
- Fluent setters enable readable construction chains
- Deprecated constructor maintains backward compatibility
- Default values (0) are appropriate for statistical counters
Also applies to: 80-89, 131-164
server/src/main/java/org/opensearch/index/warmer/ShardIndexWarmerService.java (1)
73-78: LGTM! Builder usage is correct and improves readability.The migration from positional constructor arguments to named builder methods makes the intent of each parameter clearer (current count, total count, and time in milliseconds).
server/src/test/java/org/opensearch/action/admin/indices/shards/CatShardsResponseTests.java (1)
155-164: LGTM! Test updated correctly to use Builder pattern.The explicit null assignments for optional stats fields make the test's intent clear and match the previous constructor behavior.
server/src/main/java/org/opensearch/indices/IndicesService.java (1)
892-899: LGTM! Production code correctly migrated to Builder pattern.The Builder approach significantly improves readability over the previous 7-argument constructor, especially when dealing with potentially null stats from closed shards.
server/src/test/java/org/opensearch/action/admin/cluster/stats/ClusterStatsNodesTests.java (1)
409-416: LGTM! Test helper method correctly uses Builder pattern.The Builder pattern makes the test setup more maintainable and clearer about which stats are being set versus left null.
server/src/test/java/org/opensearch/action/admin/indices/stats/IndicesStatsResponseTests.java (1)
91-100: LGTM! Test correctly updated to Builder pattern.Setting all stats to null is appropriate for this test, which focuses on index/shard aggregation logic rather than statistics content.
server/src/test/java/org/opensearch/cluster/DiskUsageTests.java (1)
138-153: LGTM! Disk usage test correctly updated to Builder pattern.The Builder approach makes it clear that this test only needs
commonStats(containing store/disk information) and doesn't require the other statistics fields.server/src/main/java/org/opensearch/action/admin/indices/stats/TransportIndicesStatsAction.java (1)
158-166: LGTM! Builder pattern correctly applied.The migration from direct constructor to
ShardStats.Builderis clean and preserves all the original field assignments. The fluent API usage is consistent with the refactoring goals.server/src/test/java/org/opensearch/action/admin/indices/rollover/TransportRolloverActionTests.java (1)
504-513: LGTM! Test code correctly updated to use Builder pattern.The
ShardStats.Builderusage in this test helper method mirrors the production code changes and maintains the same field assignments.server/src/test/java/org/opensearch/rest/action/cat/RestShardsActionTests.java (1)
90-97: LGTM! Test setup correctly migrated to Builder pattern.The
ShardStats.Builderusage maintains the same test data setup while aligning with the new construction pattern.server/src/test/java/org/opensearch/action/admin/cluster/stats/ClusterStatsResponseTests.java (1)
277-284: LGTM! Test helper correctly updated to Builder pattern.The
ShardStats.Builderusage increateShardStatsis consistent with the broader refactoring effort and maintains the same test data structure.server/src/main/java/org/opensearch/action/admin/cluster/stats/TransportClusterStatsAction.java (1)
228-236: LGTM! Builder pattern correctly applied in nodeOperation.The migration to
ShardStats.Builderpreserves the original logic, including proper handling of stats retrieved within the try-catch block forAlreadyClosedException.server/src/main/java/org/opensearch/index/IndexingPressure.java (1)
221-233: LGTM! Builder pattern correctly applied for IndexingPressureStats.The migration to
IndexingPressureStats.Builderis clean, with all 12 metrics properly mapped from the atomic counters. The fluent API improves readability compared to the previous positional constructor arguments.server/src/main/java/org/opensearch/index/stats/IndexingPressureStats.java (2)
67-85: LGTM! Well-structured Builder pattern with private constructor.The private constructor taking the Builder instance ensures that new
IndexingPressureStatsobjects are created via the Builder (or the retained deprecated constructor for backward compatibility). This is a clean implementation.
141-224: LGTM! Clean Builder implementation with proper fluent API.The Builder class correctly implements:
- All 12 fields with default values of 0
- Fluent setters returning
thisbuild()method delegating to the private constructorThis aligns well with the broader refactoring effort for stats-related classes.
server/src/test/java/org/opensearch/index/shard/IndexShardTests.java (1)
1695-1702: ShardStats builder wiring looks correctThe builder invocation passes the same shard-derived components the old constructor did (routing, path, common stats, commit/seqNo/retentionLease/pollingIngest stats), so behavior of
testShardStatsshould be preserved.
server/src/main/java/org/opensearch/action/admin/indices/stats/ShardStats.java
Show resolved
Hide resolved
There was a problem hiding this comment.
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.Builderinstead of justBuilder, or providing a brief usage hint likeShardStats.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
📒 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
Show resolved
Hide resolved
|
❌ 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? |
|
❌ 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? |
|
❌ 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? |
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
It succeeded on my local. |
Signed-off-by: Jean Kim <[email protected]>
Signed-off-by: Jean Kim <[email protected]>
Signed-off-by: Jean Kim <[email protected]>
Signed-off-by: Jean Kim <[email protected]>
Signed-off-by: Jean Kim <[email protected]>
Signed-off-by: Jean Kim <[email protected]>
pado0
force-pushed
the
builder-shard-warmer-indexingpressure
branch
from
9af35fe to
022b88e
Compare
There was a problem hiding this comment.
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. Ifbuild()is called without setting required fields (shardPath,shardRouting,commonStats), aNullPointerExceptionwill be thrown.This issue was previously flagged. Please add validation in the
Builder.build()method (lines 234-236) before constructingShardStats: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 plusmemoryLimit, so behavior matches the old all‑args constructor.- Serialization and deserialization paths (
StreamInputctor andwriteTo) 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
StreamInputctor 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
📒 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 correctThe builder invocation sets all the same fields that were previously passed to the constructor, preserving semantics including the
memoryLimitvalue fromprimaryAndCoordinatingLimits. Looks good.server/src/test/java/org/opensearch/action/admin/indices/shards/CatShardsResponseTests.java (1)
155-164: ShardStats test construction migrated cleanly to BuilderThe 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 ctorBoth 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 constructorThe 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.BuilderThe 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 onisMetricRequiredis 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 ctorThe builder setup (routing,
ShardPath, richCommonStats, 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
createShardStatsnow uses the builder to set routing, shard path, andCommonStats, 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 tonull.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
|
❌ 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? |
|
Thanks @pado0 for being patient on the last one with CI failure noise. Thanks for working through all these refactoring PRs. |
2 participants
Description
This PR refactors the
ShardStats,WarmerStatsandIndexingPressureStatsclass 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:
There are multiple stats-related classes that need similar refactoring, and we are addressing them in priority order. This PR covers
ShardStats,WarmerStatsandIndexingPressureStatsas part of that effort.Related Issues
Related to #19225
Check List
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
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.