Adding Cluster Endpoints for search failures and refactoring document status counters #19115
Conversation
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
github-actions
bot
added
enhancement
|
❌ Gradle check result for ab74705: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for ad9e107: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for 9e3af03: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for 7e9374a: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for a345b21: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for ae2c661: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for c9ad00b: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for 32a2fae: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
…g623/OpenSearch into upgrade-cluster-endpoints
|
❌ Gradle check result for 044404c: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for f15656c: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for 1f02980: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for a006f3c: TIMEOUT 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for 3c90161: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
|
❌ Gradle check result for f300d3d: 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? |
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
WalkthroughAdds a Status Counter subsystem: new StatusType enum and RestStatus.getStatusType(), LongAdder-based status-counter classes (AbstractStatusStats, DocStatusStats, SearchResponseStatusStats, StatusCounterStats), wiring into search and indices services, NodeIndicesStats/XContent changes, tests and REST spec updates with version gating. Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(240,248,255)
participant Client
participant TransportSearchAction
participant StatusUpdateListener as SearchStatusStatsUpdateListener
participant IndicesService
participant UnderlyingListener
end
Client->>TransportSearchAction: executeRequest(searchRequest, listener)
TransportSearchAction->>StatusUpdateListener: wrap(listener)
TransportSearchAction->>TransportSearchAction: doExecute(..., wrapped)
alt response
StatusUpdateListener->>IndicesService: recordSearchResponseStatus(response.status)
IndicesService->>IndicesService: statusCounterStats.add(status)
StatusUpdateListener->>UnderlyingListener: onResponse(response)
else failure/cancel
StatusUpdateListener->>IndicesService: recordSearchResponseStatus(extractedStatus)
IndicesService->>IndicesService: statusCounterStats.add(status)
StatusUpdateListener->>UnderlyingListener: onFailure(exc)
end
UnderlyingListener-->>Client: callback
sequenceDiagram
participant Client
participant NodesStatsAction
participant IndicesService
participant NodeIndicesStats
participant StatusCounterStats
Client->>NodesStatsAction: getNodesStats()
NodesStatsAction->>IndicesService: collect node indices stats
IndicesService->>StatusCounterStats: getSnapshot()
StatusCounterStats-->>IndicesService: snapshot
IndicesService->>NodeIndicesStats: include(statusCounterStats)
NodeIndicesStats-->>NodesStatsAction: serialized node stats (includes status_counter)
NodesStatsAction-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/main/java/org/opensearch/indices/NodeIndicesStats.java (1)
80-101: Ensure NodeIndicesStats streaming protocol matches presence of StatusCounterStatsThere’s a subtle wire‑compat risk here:
NodeIndicesStats(StreamInput in)unconditionally doesstatusCounterStats = new StatusCounterStats(in);whenin.getVersion().onOrAfter(Version.V_3_4_0)(Line 98‑100).writeToonly emits theStatusCounterStatspayload whenout.getVersion().onOrAfter(Version.V_3_4_0)andstatusCounterStats != null(Lines 382‑386).- The deprecated public ctors (Lines 107‑123 and 163‑192) never initialize
statusCounterStats, so any instance built that way and serialized on a 3.4+ stream will skip the field, but the reader will still try to consume it.If such an instance is ever serialized over the transport layer with a 3.4+ version, the stream will be misaligned when the reader calls into
StatusCounterStats(StreamInput), leading to incorrect parsing or runtime failures.Two concrete ways to harden this:
Guarantee non‑null statusCounterStats for all 3.4+ objects
- Initialize a default instance in all ctors that don’t receive one:
protected StatusCounterStats statusCounterStats; ... @Deprecated(since = "3.4.0") public NodeIndicesStats(CommonStats oldStats, Map<Index, List<IndexShardStats>> statsByShard, SearchRequestStats searchRequestStats) {
this.statusCounterStats = new StatusCounterStats(); this.statsByShard = statsByShard; ...- Optionally drop the `statusCounterStats != null` check in `writeTo` once it’s always initialized.
Or encode presence explicitly
Encode
StatusCounterStatsas an optional writeable, mirroring thestatsByIndex/statsByShardpattern:public NodeIndicesStats(StreamInput in) throws IOException { stats = new CommonStats(in); ... if (in.readBoolean()) { statsByShard = readStatsByShard(in); }
if (in.getVersion().onOrAfter(Version.V_3_4_0)) {statusCounterStats = new StatusCounterStats(in);}
if (in.getVersion().onOrAfter(Version.V_3_4_0) && in.readBoolean()) {statusCounterStats = new StatusCounterStats(in);} else {statusCounterStats = null;}
...
if (out.getVersion().onOrAfter(Version.V_3_4_0)) {
if (statusCounterStats != null) {statusCounterStats.writeTo(out);}
out.writeBoolean(statusCounterStats != null);if (statusCounterStats != null) {statusCounterStats.writeTo(out);}Either approach keeps the stream consistent and avoids depending on implicit invariants about how callers construct
NodeIndicesStats.Also applies to: 161-193, 200-233, 366-387
♻️ Duplicate comments (2)
server/src/main/java/org/opensearch/action/admin/indices/stats/SearchResponseStatusStats.java (1)
70-72: Consider defensive copy for exposed internal array.Same concern as in
DocStatusStats:getSearchResponseStatusCounter()exposes the internalstatusCounterarray directly. Consider returning a defensive copy if external modification is not intended.public LongAdder[] getSearchResponseStatusCounter() { - return statusCounter; + return Arrays.copyOf(statusCounter, statusCounter.length); }server/src/main/java/org/opensearch/action/admin/indices/stats/DocStatusStats.java (1)
75-81: Verify AbstractStatusStats.toXContent() endObject handling.This method calls
super.toXContent(builder, params)which ends withbuilder.endObject()inAbstractStatusStats. Ensure the object lifecycle (start/end) is properly balanced. See related comment onAbstractStatusStats.
🧹 Nitpick comments (10)
libs/core/src/main/java/org/opensearch/core/rest/StatusType.java (1)
18-27: Consider makingStatusTypea first‑class public API typeGiven this is used from
RestStatus.getStatusType(), it’s worth:
- Annotating the enum as a public API (for consistency with
RestStatus), and- Exposing
StatusTypedirectly fromRestStatus(e.g.,StatusType getStatusTypeEnum()) while usingtoString()only where a lowercase string is actually required.For example:
+import org.opensearch.common.annotation.PublicApi; ... - * @opensearch.api - */ -public enum StatusType { + * @opensearch.api + */ +@PublicApi(since = "3.4.0") +public enum StatusType {and later consider a typed accessor on
RestStatus.libs/core/src/main/java/org/opensearch/core/rest/RestStatus.java (1)
539-548: ClarifygetStatusType()contract and consider returningStatusTypeThe mapping:
- 1xx–3xx →
SUCCESS,- 4xx →
USER_ERROR,- 5xx+ →
SYSTEM_FAILURE,is reasonable, but as part of a public API it will be hard to change later. It’d be good to explicitly confirm this is the intended long‑term contract, especially for edge cases like 3xx and
MULTI_STATUS(207), which can represent partial success.Also, since there is a dedicated
StatusTypeenum now, consider providing a typed accessor such as:public StatusType getStatusTypeEnum() { ... }and using
StatusType.toString()only where a lowercase string is actually required. That keeps call sites type‑safe while preserving the current string representation for XContent/metrics.server/src/test/java/org/opensearch/index/shard/IndexingStatsTests.java (1)
57-79: IndexingStats tests align with new behavior; tiny version comment nit
- The updated XContent expectation dropping
doc_statusand includingmax_last_index_request_timestampmatches the newIndexingStats.Stats.toXContentbehavior.- The aggregation and backward‑compatibility tests for
maxLastIndexRequestTimestampcorrectly validateadd(...)and the 2.11.0 vs 3.2.0 wire gating.Minor nit: in
testMaxLastIndexRequestTimestampBackwardCompatibility, the comment says “Serialize with V_3_1_0” but the test usesVersion.V_3_2_0. Consider updating the comment (or constant if the intent was really 3.1.0) to avoid confusion.Also applies to: 94-132, 134-166, 168-183
server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java (1)
86-86: Use a spy or delegating mock forIndicesServiceto keep tests closer to production wiringRight now the tests construct a pure Mockito mock:
final IndicesService mockIndicesService = mock(IndicesService.class); when(mockIndicesService.getSearchResponseStatusStats()).thenReturn(new SearchResponseStatusStats());and pass it into
SegmentReplicationPressureService,TransportBulkAction, andTransportSearchAction. That’s sufficient to satisfy the new status‑tracking integration, but it also means any additionalIndicesServicecalls in those components will silently hit an unconfigured mock rather than the realindicesService.To keep these resiliency tests closer to real behavior and catch future regressions, consider:
final IndicesService spyIndicesService = Mockito.spy(indicesService); when(spyIndicesService.getSearchResponseStatusStats()).thenReturn(new SearchResponseStatusStats()); // then pass spyIndicesService everywhere mockIndicesService is currently used(or a similarly delegating mock). That preserves real
IndicesServicebehavior while still allowing you to plug inSearchResponseStatusStatsfor the new path.Also applies to: 2259-2261, 2272-2279, 2285-2312, 2380-2412
server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java (1)
121-162: Consider using XContentBuilder comparison instead of string matching.Manually constructing the expected JSON string is brittle and error-prone. Consider parsing both the expected and actual XContent to maps and comparing them, or use
XContentTestUtils.differenceBetweenMapsIgnoringArrayOrder.Alternative approach:
public void testToXContentForStatusCounterStats() throws IOException { StatusCounterStats statusCounterStats = createStatusCounters(); XContentBuilder builder = MediaTypeRegistry.contentBuilder(MediaTypeRegistry.JSON); builder.startObject(); statusCounterStats.toXContent(builder, ToXContent.EMPTY_PARAMS); builder.endObject(); Map<String, Object> actual = XContentHelper.convertToMap( BytesReference.bytes(builder), false, builder.contentType() ).v2(); // Verify structure programmatically assertNotNull(actual.get("status_counter")); Map<String, Object> statusCounter = (Map<String, Object>) actual.get("status_counter"); assertNotNull(statusCounter.get("doc_status")); assertNotNull(statusCounter.get("search_response_status")); }server/src/main/java/org/opensearch/action/admin/indices/stats/AbstractStatusStats.java (1)
32-48: Document the array size assumption.The hard-coded size of 5 for
statusCounterarray assumes exactly 5 HTTP status families (1xx-5xx). This should be documented to clarify the design constraint.Add a comment:
public AbstractStatusStats() { + // Array size 5 corresponds to the 5 HTTP status families: 1xx, 2xx, 3xx, 4xx, 5xx statusCounter = new LongAdder[5]; for (int i = 0; i < statusCounter.length; ++i) { statusCounter[i] = new LongAdder(); } }server/src/main/java/org/opensearch/action/admin/indices/stats/DocStatusStats.java (1)
71-73: Consider defensive copy for exposed internal array.
getDocStatusCounter()returns a direct reference to the internalstatusCounterarray. Callers could modify the counters directly, bypassing theadd()methods and potentially causing concurrency issues.If external modification is not intended, consider returning a defensive copy or making the array unmodifiable:
public LongAdder[] getDocStatusCounter() { - return statusCounter; + return Arrays.copyOf(statusCounter, statusCounter.length); }Alternatively, if external access is intentional for performance reasons (e.g., for test assertions), document this in a Javadoc comment.
server/src/main/java/org/opensearch/action/admin/indices/stats/StatusCounterStats.java (1)
46-58: Tighten version/optional handling to avoid future NPEsThe StreamInput ctor and
writeTo/toXContentall assume inner stats are non‑null, but the fields are@Nullableand the ctor explicitly assignsnullfor pre‑3.4.0 versions. Today this is probably safe because only 3.4+ writers ever sendStatusCounterStats, but as a public type it’s easy for external code to construct or deserialize instances with null internals.To make the class robust without changing semantics:
public StatusCounterStats(StreamInput in) throws IOException { - if (in.getVersion().onOrAfter(Version.V_3_4_0)) { - docStatusStats = in.readOptionalWriteable(DocStatusStats::new); - } else { - docStatusStats = null; - } - if (in.getVersion().onOrAfter(Version.V_3_4_0)) { - searchResponseStatusStats = in.readOptionalWriteable(SearchResponseStatusStats::new); - } else { - searchResponseStatusStats = null; - } + if (in.getVersion().onOrAfter(Version.V_3_4_0)) { + docStatusStats = in.readOptionalWriteable(DocStatusStats::new); + searchResponseStatusStats = in.readOptionalWriteable(SearchResponseStatusStats::new); + } else { + docStatusStats = null; + searchResponseStatusStats = null; + } }and in writers:
if (out.getVersion().onOrAfter(Version.V_3_4_0)) { - out.writeOptionalWriteable(docStatusStats.getSnapshot()); + out.writeOptionalWriteable(docStatusStats == null ? null : docStatusStats.getSnapshot()); } if (out.getVersion().onOrAfter(Version.V_3_4_0)) { - out.writeOptionalWriteable(searchResponseStatusStats.getSnapshot()); + out.writeOptionalWriteable( + searchResponseStatusStats == null ? null : searchResponseStatusStats.getSnapshot() + ); }Similarly, guard
toXContent:builder.startObject(Fields.STATUS_COUNTER); -if (docStatusStats != null) { - docStatusStats.getSnapshot().toXContent(builder, params); -} -if (searchResponseStatusStats != null) { - searchResponseStatusStats.getSnapshot().toXContent(builder, params); -} +if (docStatusStats != null) { + docStatusStats.getSnapshot().toXContent(builder, params); +} +if (searchResponseStatusStats != null) { + searchResponseStatusStats.getSnapshot().toXContent(builder, params); +} builder.endObject();This keeps the wire/XContent shape the same when stats are present, but avoids hard assumptions about non‑null internals.
Also applies to: 82-101
server/src/main/java/org/opensearch/indices/NodeIndicesStats.java (1)
125-153: Prefer passing a snapshot StatusCounterStats into NodeIndicesStatsThe new ctors (Lines 129‑153, 200‑233) store the given
StatusCounterStatsinstance directly:this.statusCounterStats = statusCounterStats;and
getStatusCounterStats()exposes it (Lines 361‑364). The comment above the 4‑arg ctor explicitly says:statusCounterStats should be a snapshot of the statusCounters at a point in time, just like all the items in NodeIndicesStats should be.
However,
IndicesService.stats(...)currently passes the live accumulator instance fromIndicesServicerather than a snapshot. That means:
NodeIndicesStatsdoesn’t truly represent a frozen point‑in‑time view of status counters.- Any external code that (incorrectly) mutates
getStatusCounterStats()from aNodeIndicesStatsinstance would be mutating the shared accumulator.A cleaner pattern is:
- Treat
StatusCounterStatsinNodeIndicesStatsas an immutable snapshot.- Have the caller pass
statusCounterStats.getSnapshot()into the ctor, and consider documenting or enforcing non‑null viaObjects.requireNonNull.Example change on the caller side (IndicesService):
- StatusCounterStats statusCounterStats = this.statusCounterStats; + StatusCounterStats statusCounterStats = this.statusCounterStats.getSnapshot(); ... return new NodeIndicesStats(commonStats, statsByShard(this, flags), searchRequestStats, statusCounterStats, statsLevel);and keep the ctor as is. This aligns the implementation with the Javadoc and avoids leaking a mutable accumulator through a stats snapshot object.
Also applies to: 194-233, 361-365, 432-438
server/src/main/java/org/opensearch/indices/IndicesService.java (1)
416-424: Use a snapshot when exporting status counters via NodeIndicesStats
IndicesServicenow owns a singletonStatusCounterStatsaccumulator (Line 423) and passes it directly intoNodeIndicesStats(Lines 808‑842):return new NodeIndicesStats(commonStats, statsByShard(this, flags), searchRequestStats, statusCounterStats, statsLevel);At the same time, the new
NodeIndicesStatsctor comments say the embeddedstatusCounterStats“should be a snapshot of the statusCounters at a point in time”.For clearer semantics and less risk of accidental mutation via
NodeIndicesStats.getStatusCounterStats(), consider passing a snapshot instead:StatusCounterStats snapshot = statusCounterStats.getSnapshot(); if (flags.getIncludeIndicesStatsByLevel()) { - return new NodeIndicesStats(commonStats, statsByShard(this, flags), searchRequestStats, statusCounterStats, statsLevel); + return new NodeIndicesStats(commonStats, statsByShard(this, flags), searchRequestStats, snapshot, statsLevel); } else { - return new NodeIndicesStats(commonStats, statsByShard(this, flags), searchRequestStats, statusCounterStats); + return new NodeIndicesStats(commonStats, statsByShard(this, flags), searchRequestStats, snapshot); }The accumulator remains mutable inside
IndicesService(foraddDocStatusStatsandgetSearchResponseStatusStats()), but exportedNodeIndicesStatsinstances then behave like proper point‑in‑time views.You might also want a small null‑guard on
addDocStatusStats(DocStatusStats stats)to be robust against callers passing null.Also applies to: 808-842, 1391-1417
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
CHANGELOG.md(1 hunks)libs/core/src/main/java/org/opensearch/core/rest/RestStatus.java(1 hunks)libs/core/src/main/java/org/opensearch/core/rest/StatusType.java(1 hunks)rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/11_indices_metrics.yml(7 hunks)server/src/internalClusterTest/java/org/opensearch/nodestats/NodeStatsIT.java(4 hunks)server/src/internalClusterTest/java/org/opensearch/search/msearch/MultiSearchStatsIT.java(1 hunks)server/src/main/java/org/opensearch/action/admin/indices/stats/AbstractStatusStats.java(1 hunks)server/src/main/java/org/opensearch/action/admin/indices/stats/DocStatusStats.java(1 hunks)server/src/main/java/org/opensearch/action/admin/indices/stats/SearchResponseStatusStats.java(1 hunks)server/src/main/java/org/opensearch/action/admin/indices/stats/StatusCounterStats.java(1 hunks)server/src/main/java/org/opensearch/action/bulk/TransportBulkAction.java(1 hunks)server/src/main/java/org/opensearch/action/search/StreamTransportSearchAction.java(3 hunks)server/src/main/java/org/opensearch/action/search/TransportSearchAction.java(6 hunks)server/src/main/java/org/opensearch/action/update/TransportUpdateAction.java(1 hunks)server/src/main/java/org/opensearch/index/shard/IndexingStats.java(8 hunks)server/src/main/java/org/opensearch/index/shard/InternalIndexingStats.java(0 hunks)server/src/main/java/org/opensearch/indices/IndicesService.java(5 hunks)server/src/main/java/org/opensearch/indices/NodeIndicesStats.java(8 hunks)server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java(6 hunks)server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java(2 hunks)server/src/test/java/org/opensearch/core/RestStatusTests.java(3 hunks)server/src/test/java/org/opensearch/index/shard/IndexingStatsTests.java(3 hunks)server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java(3 hunks)server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java(5 hunks)
💤 Files with no reviewable changes (1)
- server/src/main/java/org/opensearch/index/shard/InternalIndexingStats.java
🧰 Additional context used
🧬 Code graph analysis (5)
server/src/main/java/org/opensearch/action/admin/indices/stats/SearchResponseStatusStats.java (2)
server/src/main/java/org/opensearch/action/admin/indices/stats/StatusCounterStats.java (2)
PublicApi(27-111)Fields(108-110)server/src/main/java/org/opensearch/action/admin/indices/stats/DocStatusStats.java (2)
PublicApi(25-105)Fields(102-104)
server/src/main/java/org/opensearch/action/admin/indices/stats/DocStatusStats.java (2)
server/src/main/java/org/opensearch/action/admin/indices/stats/StatusCounterStats.java (2)
PublicApi(27-111)Fields(108-110)server/src/main/java/org/opensearch/action/admin/indices/stats/SearchResponseStatusStats.java (2)
PublicApi(25-104)Fields(101-103)
server/src/main/java/org/opensearch/action/admin/indices/stats/StatusCounterStats.java (2)
server/src/main/java/org/opensearch/index/shard/IndexingStats.java (4)
PublicApi(57-591)PublicApi(65-510)PublicApi(73-151)Fields(564-581)server/src/main/java/org/opensearch/action/admin/indices/stats/SearchResponseStatusStats.java (2)
PublicApi(25-104)Fields(101-103)
server/src/main/java/org/opensearch/action/search/TransportSearchAction.java (1)
libs/core/src/main/java/org/opensearch/ExceptionsHelper.java (1)
ExceptionsHelper(76-476)
server/src/main/java/org/opensearch/index/shard/IndexingStats.java (1)
server/src/main/java/org/opensearch/script/ScriptCacheStats.java (1)
DeprecatedApi(56-181)
⏰ 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 (21, macos-15)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
🔇 Additional comments (14)
CHANGELOG.md (1)
35-35: Verify PR reference number in changelog entry.The changelog entry references PR #18601, but this change is being merged as PR #19115 (a rebased and cleaned version of #18601). Verify whether the changelog should reference the current PR (#19115) or if referencing the original PR (#18601) is intentional.
server/src/main/java/org/opensearch/action/update/TransportUpdateAction.java (1)
42-42: DocStatusStats import correctly updated to new stats packagePointing
DocStatusStatstoorg.opensearch.action.admin.indices.stats.DocStatusStatskeeps the existing NOOP and error tracking logic intact while aligning this action with the refactored coordinator‑level stats model; no behavior change in this class.server/src/main/java/org/opensearch/index/shard/IndexingStats.java (1)
73-75: DocStatusStats deprecation and wire gating look soundThe combination of:
- Marking
DocStatusStatsas@DeprecatedApi(since = "3.4.0"),- Defaulting
Stats()todocStatusStats = null,- Reading/writing
docStatusStatsonly for2.11.0 ≤ version < 3.4.0, and- Guarding aggregation/XContent on
getDocStatusStats() != nullgives correct BWC behavior (2.11–3.3 interop) while cleanly dropping
doc_statusfrom 3.4+ stats output. No changes needed from my side here.Also applies to: 166-168, 190-211, 217-278, 368-370, 376-394, 491-495
server/src/main/java/org/opensearch/action/bulk/TransportBulkAction.java (1)
49-50: Ensure newDocStatusStatsimplementation is safe for concurrent bulk usage
BulkOperationmaintains a singleDocStatusStatsinstance (docStatusStats) and updates it from multiple shard response/failure callbacks, potentially on different threads:
- Lines 682–687 (empty
requestsByShardpath),- Lines 733–735 and 754–756 in the shard response/failure handlers.
With the switch to
org.opensearch.action.admin.indices.stats.DocStatusStats, please double‑check that its counters remain thread‑safe (e.g., still based onAtomicLong,LongAdder, orMetric) so these concurrentinc(...)calls can’t race or corrupt counts. If it’s no longer thread‑safe, we’d need per‑shard instances or internal synchronization around updates.Also applies to: 682-689, 694-765
server/src/test/java/org/opensearch/core/RestStatusTests.java (1)
16-17: Good coverage ofgetStatusType()behaviorThe added assertions nicely exercise the new
RestStatus.getStatusType()contract across:
- All‑successful shards,
- All‑unavailable shards, and
- The “highest failure status” aggregation path.
This should catch regressions in both the family‑code mapping and the string representation.
Also applies to: 29-31, 37-39, 61-67
server/src/main/java/org/opensearch/action/search/StreamTransportSearchAction.java (1)
21-22: Constructor update cleanly forwardsIndicesServiceto the base classThe added
IndicesServiceparameter is just threaded through toTransportSearchActionwithout altering any subclass behavior, keeping streaming search aligned with the main search action’s new constructor. Looks good.Also applies to: 45-63, 64-82
server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java (1)
79-79: LGTM! Test setup correctly updated.The test properly adds
IndicesServicemock to align with the production changes inTransportSearchActionconstructor.Also applies to: 1249-1250
server/src/test/java/org/opensearch/indices/NodeIndicesStatsTests.java (1)
77-119: Good test coverage for serialization.The test properly validates round-trip serialization of
StatusCounterStatsand verifies counter values using deep equality withLongAddercomparisons.server/src/internalClusterTest/java/org/opensearch/nodestats/NodeStatsIT.java (1)
16-16: LGTM! Correctly updated to use new StatusCounterStats API.The changes properly migrate from the old
getIndexing().getTotal()path to the newgetStatusCounterStats().getDocStatusStats()path, and update the comparator to useLongAdderinstead ofAtomicLong, which is more appropriate for high-contention concurrent counters.Also applies to: 50-50, 284-292, 545-553
server/src/main/java/org/opensearch/action/admin/indices/stats/SearchResponseStatusStats.java (1)
26-104: Good implementation addressing past review feedback.The introduction of
AbstractStatusStatsas a base class successfully addresses the duplication concern raised in past reviews. The common functionality is now properly factored into the abstract base class, with only the specific field names and XContent structure differing betweenDocStatusStatsandSearchResponseStatusStats.rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/11_indices_metrics.yml (1)
4-6: Let me retry the shell script to access the repository:
<function_calls>
#!/bin/bashFirst, check if we're already in a repository
pwd
git remote -v 2>/dev/null || echo "Not in git repository"List files to understand structure
ls -la | head -20
</function_calls>server/src/main/java/org/opensearch/action/admin/indices/stats/AbstractStatusStats.java (1)
65-67: Based on my investigation, I found critical information but cannot access the repository to fully verify the concern:What I verified:
RestStatus.getStatusFamilyCode()returnsstatus / 100(integer division)- For valid HTTP status codes (100-599), this produces values 1-5
- Array access formula:
statusCounter[status.getStatusFamilyCode() - 1]maps 1-5 to indices 0-4What I cannot verify:
- The actual
statusCounterarray size inAbstractStatusStats- Whether the method signature enforces
RestStatusenum type (which would guarantee values 1-5)- Whether there are input validation guards elsewhere in the code
Key finding: While
RestStatusenum values would produce safe results (1-5), the mathematical formulastatus / 100could theoretically produce 0 or 6+ if invalid status codes were passed. However, if the method parameter is typed asRestStatus(enum), Java's type system prevents this.</blockquote></details> <details> <summary>server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java (1)</summary><blockquote> `40-41`: **Wiring StatusCounterStats into NodeIndicesStats tests looks consistent** All test helpers and MockNodeIndicesStats constructors now pass a `StatusCounterStats` instance through to `NodeIndicesStats`, and the old-version serialization test still exercises the legacy path via explicit stream version settings. No functional issues spotted here. Also applies to: 1057-1066, 1121-1138, 1329-1335, 1339-1345, 1495-1507 </blockquote></details> <details> <summary>server/src/main/java/org/opensearch/action/search/TransportSearchAction.java (1)</summary><blockquote> `35-36`: **Search response status tracking via IndicesService is wired correctly** The new `IndicesService` dependency and the `searchStatusStatsUpdateListener` in `doExecute` cleanly: - Wrap both the timeout‑cancellation listener and the plain listener without changing their observable behavior. - Increment the shared `SearchResponseStatusStats` once per search request, on both success (`searchResponse.status()`) and failure (`ExceptionsHelper.status(e)`). - Ensure counter updates happen even if the downstream listener throws, thanks to the try/finally structure. This looks sound and matches the intended coordinator‑node tracking behavior. Also applies to: 75-76, 186-187, 192-235, 320-366 </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
server/src/internalClusterTest/java/org/opensearch/search/msearch/MultiSearchStatsIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/opensearch/search/msearch/MultiSearchStatsIT.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/action/admin/indices/stats/AbstractStatusStats.java
Show resolved
Hide resolved
server/src/main/java/org/opensearch/action/admin/indices/stats/StatusCounterStats.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
server/src/internalClusterTest/java/org/opensearch/search/msearch/MultiSearchStatsIT.java (2)
29-59: Rename test method to reflect multi-search/SearchResponseStatusStats behaviorThe method name
testNodeIndicesStatsDocStatusStatsIndexBulkno longer matches what the test does (multi-search requests andSearchResponseStatusStatson search responses, not bulk/doc status). Consider renaming to something liketestNodeIndicesStatsSearchResponseStatusStatsMultiSearchto keep tests self‑describing and aligned with the new stats type.
61-78: Assert the result ofArrays.equalsso the test actually validates stats
Arrays.equals(...)returns a boolean that is currently ignored, so the test will pass even if the actual and expected counters differ. Wrap this in an assertion (e.g.,assertTrue(...)) so a mismatch fails the test.- Arrays.equals( - searchResponseStatusStats.getSearchResponseStatusCounter(), - expectedSearchResponseStatusStats.getSearchResponseStatusCounter(), - Comparator.comparingLong(LongAdder::longValue) - ); + assertTrue( + Arrays.equals( + searchResponseStatusStats.getSearchResponseStatusCounter(), + expectedSearchResponseStatusStats.getSearchResponseStatusCounter(), + Comparator.comparingLong(LongAdder::longValue) + ) + );
🧹 Nitpick comments (2)
server/src/internalClusterTest/java/org/opensearch/search/msearch/MultiSearchStatsIT.java (1)
80-86: Align helper naming/signature withSearchResponseStatusStatsusageBoth
updateExpectedDocStatusCounter(...)overloads operate onSearchResponseStatusStatsand always receiveexpectedSearchResponseStatusStats; theDocStatuswording and explicit parameter are legacy artifacts and slightly confusing. Consider renaming toupdateExpectedSearchResponseStatusCounterand either using the field directly or making the method static and clearly generic.server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java (1)
1339-1345: Aggregated NodeIndicesStats test wiring also includes status countersThe aggregated
MockNodeIndicesStatspath similarly receives aStatusCounterStats, keeping both code paths structurally equivalent for the aggregation vs. non‑aggregation comparison.If you later want stronger coverage for the new counters, consider pre‑populating
StatusCounterStatswith non‑zero values in one of these constructions and asserting they round‑trip throughtoXContentas expected (assuming the API exposes a safe increment method).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/internalClusterTest/java/org/opensearch/search/msearch/MultiSearchStatsIT.java(1 hunks)server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/internalClusterTest/java/org/opensearch/search/msearch/MultiSearchStatsIT.java (2)
libs/core/src/main/java/org/opensearch/ExceptionsHelper.java (1)
ExceptionsHelper(76-476)server/src/main/java/org/opensearch/index/query/QueryBuilders.java (1)
QueryBuilders(60-804)
⏰ 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: Analyze (java)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
🔇 Additional comments (7)
server/src/test/java/org/opensearch/action/admin/cluster/node/stats/NodeStatsTests.java (7)
35-42: ImportingStatusCounterStatsis appropriateThe new import cleanly matches the later usages in constructor calls; no issues here.
1057-1066: NodeIndicesStats construction updated correctly for new signatureProviding both
SearchRequestStatsand a freshStatusCounterStatswhenremoteStoreStatsis enabled aligns with the extendedNodeIndicesStatsconstructor and ensures the new field is present duringNodeStatsserialization tests.
1121-1127: MockNodeIndicesStats 3‑arg ctor correctly forwards new status countersAdding
StatusCounterStatsto the constructor and forwarding it tosuperkeeps the mock in sync with the productionNodeIndicesStatsAPI.
1130-1138: MockNodeIndicesStats 4‑arg ctor (with level) remains consistentThe overload that includes
StatsLevelnow also acceptsStatusCounterStatsand passes it through to the base class, which is consistent with the updatedNodeIndicesStatsconstructor set.
1329-1334: Non‑aggregated NodeIndicesStats test wiring includes status counters
testNodeIndicesStatsWithAndWithoutAggregationsnow constructs the non‑aggregatedMockNodeIndicesStatswith aStatusCounterStatsinstance, so the new field participates in the equivalence check setup.
1497-1503: generateMockNodeIndicesStats (by‑level branch) passes status counters throughWhen
includeIndicesStatsByLevelis true, the helper now passes aStatusCounterStatsintoMockNodeIndicesStats, ensuring that even pre‑aggregated stats objects are constructed with the new field initialized.
1505-1510: generateMockNodeIndicesStats (non by‑level branch) remains BWC‑focused and correctFor the non‑by‑level path used by BWC tests, returning a
MockNodeIndicesStatsthat includesStatusCounterStatskeeps serialization/deserialization tests aligned with the updatedNodeIndicesStatsshape without changing existing assertions.
Signed-off-by: Anthony Leong <aj.leong623@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
server/src/main/java/org/opensearch/action/admin/indices/stats/StatusCounterStats.java (1)
90-99: Potential NPE when serializing with null fields.Both
docStatusStatsandsearchResponseStatusStatsare@Nullableand can be null after deserialization from older versions (< 3.4.0). CallinggetSnapshot()on a null field will throwNullPointerException.@Override public void writeTo(StreamOutput out) throws IOException { if (out.getVersion().onOrAfter(Version.V_3_4_0)) { - out.writeOptionalWriteable(docStatusStats.getSnapshot()); - } - - if (out.getVersion().onOrAfter(Version.V_3_4_0)) { - out.writeOptionalWriteable(searchResponseStatusStats.getSnapshot()); + out.writeOptionalWriteable(docStatusStats != null ? docStatusStats.getSnapshot() : null); + out.writeOptionalWriteable(searchResponseStatusStats != null ? searchResponseStatusStats.getSnapshot() : null); } }
🧹 Nitpick comments (1)
server/src/main/java/org/opensearch/action/admin/indices/stats/StatusCounterStats.java (1)
46-58: Consider consolidating duplicate version checks.Lines 47 and 53 check the same condition
in.getVersion().onOrAfter(Version.V_3_4_0). These could be combined into a single block.public StatusCounterStats(StreamInput in) throws IOException { - if (in.getVersion().onOrAfter(Version.V_3_4_0)) { - docStatusStats = in.readOptionalWriteable(DocStatusStats::new); - } else { - docStatusStats = null; - } - - if (in.getVersion().onOrAfter(Version.V_3_4_0)) { - searchResponseStatusStats = in.readOptionalWriteable(SearchResponseStatusStats::new); + if (in.getVersion().onOrAfter(Version.V_3_4_0)) { + docStatusStats = in.readOptionalWriteable(DocStatusStats::new); + searchResponseStatusStats = in.readOptionalWriteable(SearchResponseStatusStats::new); } else { + docStatusStats = null; searchResponseStatusStats = null; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/src/internalClusterTest/java/org/opensearch/search/msearch/MultiSearchStatsIT.java(1 hunks)server/src/main/java/org/opensearch/action/admin/indices/stats/AbstractStatusStats.java(1 hunks)server/src/main/java/org/opensearch/action/admin/indices/stats/DocStatusStats.java(1 hunks)server/src/main/java/org/opensearch/action/admin/indices/stats/SearchResponseStatusStats.java(1 hunks)server/src/main/java/org/opensearch/action/admin/indices/stats/StatusCounterStats.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- server/src/internalClusterTest/java/org/opensearch/search/msearch/MultiSearchStatsIT.java
- server/src/main/java/org/opensearch/action/admin/indices/stats/AbstractStatusStats.java
- server/src/main/java/org/opensearch/action/admin/indices/stats/SearchResponseStatusStats.java
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/main/java/org/opensearch/action/admin/indices/stats/DocStatusStats.java (2)
server/src/main/java/org/opensearch/action/admin/indices/stats/SearchResponseStatusStats.java (2)
PublicApi(25-104)Fields(101-103)server/src/main/java/org/opensearch/action/admin/indices/stats/StatusCounterStats.java (2)
PublicApi(27-119)Fields(116-118)
⏰ 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: precommit (25, ubuntu-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, 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 (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: Analyze (java)
🔇 Additional comments (7)
server/src/main/java/org/opensearch/action/admin/indices/stats/DocStatusStats.java (5)
28-34: LGTM!Constructors properly delegate to the parent class, consistent with the pattern in
SearchResponseStatusStats.
41-54: LGTM, but override onadd()is redundant.The
add(RestStatus, long)method only delegates tosuper.add()without additional logic. The@Overridecould be removed unless the Javadoc documentation is specifically needed here.
61-73: LGTM!Null-safety is properly handled in the aggregation method, and the implementation is consistent with
SearchResponseStatusStats.
75-95: LGTM!The serialization and snapshot methods follow the established pattern from
SearchResponseStatusStatsand correctly produce point-in-time snapshots.
97-104: LGTM!Standard pattern for XContent field name constants.
server/src/main/java/org/opensearch/action/admin/indices/stats/StatusCounterStats.java (2)
78-88: LGTM!The
add()method now correctly handles null-safety for both the parameter and the internal nullable fields, addressing the previous review feedback.
71-76: LGTM!The
getSnapshot()implementation is safe because the default constructor initializes non-null fields, and the nestedadd()methods properly handle null parameters.
| @PublicApi(since = "1.0.0") | ||
| public class DocStatusStats extends AbstractStatusStats { |
There was a problem hiding this comment.
Incorrect @PublicApi version annotation.
This is a new class introduced in this PR (targeting 3.4.0), but the annotation claims since = "1.0.0". This misrepresents API stability guarantees.
-@PublicApi(since = "1.0.0")
+@PublicApi(since = "3.4.0")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @PublicApi(since = "1.0.0") | |
| public class DocStatusStats extends AbstractStatusStats { | |
| @PublicApi(since = "3.4.0") | |
| public class DocStatusStats extends AbstractStatusStats { |
🤖 Prompt for AI Agents
In
server/src/main/java/org/opensearch/action/admin/indices/stats/DocStatusStats.java
around lines 25-26, the @PublicApi annotation incorrectly states since =
"1.0.0"; update the annotation to reflect the actual introduction version for
this PR (since = "3.4.0") so the API stability metadata is accurate — change the
string literal in the @PublicApi(since = "...") to "3.4.0".
| @PublicApi(since = "1.0.0") | ||
| public class StatusCounterStats implements Writeable, ToXContentFragment { |
There was a problem hiding this comment.
Incorrect @PublicApi version annotation.
This is a new class introduced in this PR (targeting 3.4.0), but the annotation claims since = "1.0.0".
-@PublicApi(since = "1.0.0")
+@PublicApi(since = "3.4.0")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @PublicApi(since = "1.0.0") | |
| public class StatusCounterStats implements Writeable, ToXContentFragment { | |
| @PublicApi(since = "3.4.0") | |
| public class StatusCounterStats implements Writeable, ToXContentFragment { |
🤖 Prompt for AI Agents
In
server/src/main/java/org/opensearch/action/admin/indices/stats/StatusCounterStats.java
around lines 27-28 the @PublicApi annotation incorrectly declares since =
"1.0.0" for a class introduced in this PR; update the annotation to reflect the
correct release by changing the since value to "3.4.0" (i.e., @PublicApi(since =
"3.4.0")), ensuring the file compiles after the change.
| @Override | ||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
| builder.startObject(Fields.STATUS_COUNTER); | ||
| docStatusStats.getSnapshot().toXContent(builder, params); | ||
| searchResponseStatusStats.getSnapshot().toXContent(builder, params); | ||
| builder.endObject(); | ||
|
|
||
| return builder; | ||
| } |
There was a problem hiding this comment.
Potential NPE in toXContent() with null fields.
Same issue as writeTo() — calling getSnapshot() on null docStatusStats or searchResponseStatusStats will throw NullPointerException.
@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(Fields.STATUS_COUNTER);
- docStatusStats.getSnapshot().toXContent(builder, params);
- searchResponseStatusStats.getSnapshot().toXContent(builder, params);
+ if (docStatusStats != null) {
+ docStatusStats.getSnapshot().toXContent(builder, params);
+ }
+ if (searchResponseStatusStats != null) {
+ searchResponseStatusStats.getSnapshot().toXContent(builder, params);
+ }
builder.endObject();
return builder;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | |
| builder.startObject(Fields.STATUS_COUNTER); | |
| docStatusStats.getSnapshot().toXContent(builder, params); | |
| searchResponseStatusStats.getSnapshot().toXContent(builder, params); | |
| builder.endObject(); | |
| return builder; | |
| } | |
| @Override | |
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | |
| builder.startObject(Fields.STATUS_COUNTER); | |
| if (docStatusStats != null) { | |
| docStatusStats.getSnapshot().toXContent(builder, params); | |
| } | |
| if (searchResponseStatusStats != null) { | |
| searchResponseStatusStats.getSnapshot().toXContent(builder, params); | |
| } | |
| builder.endObject(); | |
| return builder; | |
| } |
🤖 Prompt for AI Agents
In
server/src/main/java/org/opensearch/action/admin/indices/stats/StatusCounterStats.java
around lines 101–109, toXContent() calls getSnapshot() on docStatusStats and
searchResponseStatusStats without null checks which can cause NPEs; add null
guards for both fields before calling getSnapshot() (e.g., if (docStatusStats !=
null) { docStatusStats.getSnapshot().toXContent(builder, params); } else { /*
skip or write empty/placeholder as done in writeTo() */ }) and do the same for
searchResponseStatusStats so the builder remains well-formed when either field
is null.
|
❕ Gradle check result for b8e3881: 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. |
4 participants
Description
This change is a rebased and cleaned version of another pr #18601
Related Issues
Resolves #18377, resolves #18438
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
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.