Skip to content

Fix ShardProfile.fetch typed as single FetchProfile instead of List<FetchProfile> - #1197

Open
gingeekrishna wants to merge 2 commits into
opensearch-project:mainfrom
gingeekrishna:fix/shardprofile-fetch-array
Open

Fix ShardProfile.fetch typed as single FetchProfile instead of List<FetchProfile>#1197
gingeekrishna wants to merge 2 commits into
opensearch-project:mainfrom
gingeekrishna:fix/shardprofile-fetch-array

Conversation

@gingeekrishna

Copy link
Copy Markdown

ShardProfile.fetch is currently typed as a single $ref: FetchProfile, but the OpenSearch server returns an array of FetchProfile objects. This fixes the type to type: array, items: $ref: FetchProfile.

Related: opensearch-project/opensearch-java#2040

Signed-off-by: Radhakrishnan P gingeekrishna@gmail.com

Signed-off-by: Radhakrishnan Pachyappan <radhakrishnan.p@op.tech>
Copilot AI lite review requested due to automatic review settings August 17, 2026 03:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@iprithv iprithv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@minseoky

Copy link
Copy Markdown

Hi @gingeekrishna, thanks for picking this up, and apologies for coming into your thread late.

I'd like to flag a duplication concern: #1147, opened on June 11, fixes the same bug (tracked from opensearch-project/opensearch-java#1965). I'm raising this only because I'd be sorry to see the fix land partially, not to claim any priority — I'd much rather this gets fixed than that it gets fixed by me.

The two differ in scope. This PR corrects ShardProfile.fetch to an array. #1147 does the same and also covers two further mismatches that surfaced once I added a spec test for profile: true:

  • ShardProfile is missing inbound_network_time_in_millis and outbound_network_time_in_millis, which the server has written unconditionally since 2.0
  • FetchProfileBreakdown doesn't match the server's FetchTimingType: four fields are missing, next_reader is never emitted, and the values are long rather than int32

These two are prerequisites for the test rather than optional extras — the spec test harness validates the entire response payload, so a profile: true test cannot pass with the fetch fix alone.

My concern with merging the array change on its own is that the schema would still not match what the server actually sends, and nothing would guard it, so I'd expect the remaining mismatches to come back as fresh client deserialization reports.

That said, I'm happy either way. If maintainers prefer the smaller footprint here, I'm glad to rebase #1147 to drop the overlapping change and keep only the remaining fixes plus the test. @Xtansia @VachaShah — would you mind advising which route you'd prefer?

@gingeekrishna

Copy link
Copy Markdown
Author

Thanks for flagging this, @minseoky — no need to apologize, I appreciate you catching it.

Looking at #1147, it's clearly more thorough than this PR: it fixes the same fetch-as-array bug, but also covers the inbound_network_time_in_millis/outbound_network_time_in_millis gap and the FetchProfileBreakdown/FetchTimingType mismatch, and has a spec test that validates the full profile: true payload (which, as you note, this PR alone wouldn't pass).

I don't want to land a partial fix that leaves the schema still mismatched, so I'm fine deferring to #1147's broader scope. I'll leave the call on which PR to merge to @Xtansia / @VachaShah, and I'm happy to close this one in favor of #1147 if that's their preference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants