Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed swapped schema references in nodes info API buffer fields #808

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hye-on
Copy link
Contributor

@hye-on hye-on commented Jan 25, 2025

Description

Fixed incorrectly referenced schemas in nodes info API buffer fields by swapping the schema types between total_indexing_buffer and total_indexing_buffer_in_bytes .

Issues Resolved

Related to opensearch-project/OpenSearch#16910

Related PR

opensearch-project/OpenSearch#17070

Comments

This change will be effective from version 3.0.0 onwards

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.

Copy link
Contributor

Changes Analysis

Commit SHA: 0582674
Comparing To SHA: 592336a

API Changes

Summary

└─┬Components
  └─┬nodes.info___NodeInfo
    ├─┬total_indexing_buffer
    │ └──[🔀] $ref (35025:13)❌ 
    └─┬total_indexing_buffer_in_bytes
      └──[🔀] $ref (34425:13)❌ 

Document Element Total Changes Breaking Changes
components 2 2
  • BREAKING Changes: 2 out of 2
  • Modifications: 2
  • Breaking Modifications: 2

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/12963024890/artifacts/2484673222

API Coverage

Before After Δ
Covered (%) 662 (64.84 %) 662 (64.84 %) 0 (0 %)
Uncovered (%) 359 (35.16 %) 359 (35.16 %) 0 (0 %)
Unknown 45 45 0

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

@Xtansia I read the related issue, this is correct right?

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Actually we now have a failing test. The spec needs to work correctly for all versions, how do we fix this?

@hye-on
Copy link
Contributor Author

hye-on commented Jan 26, 2025

Actually we now have a failing test. The spec needs to work correctly for all versions, how do we fix this?

@dblock I noticed that PR opensearch-project/OpenSearch#17070 will implement changes from OpenSearch 3.0.0. Can the api-specification repository define different specifications per version? Or should I wait until OpenSearch 3.0.0 is released to submit this PR?

I've reviewed the failing test files and related files (tests/default/nodes/nodes.yaml, tests/default/nodes/usage.yaml, json_schemas/test_story.schema.yaml, and others) but couldn't identify where to make changes. (Does the test payload response come from OpenSearch itself?) If I've missed any files that should be reviewed, I would greatly appreciate your guidance.

Thank you for taking the time to review this and for your help! :)

@dblock
Copy link
Member

dblock commented Jan 26, 2025

@hye-on In general we would do this:

  1. Annotate the current schema for both fields with x-version-removed: 3.0.
  2. Add the new schema with x-version-added: 3.0.

See https://github.com/opensearch-project/opensearch-api-specification/blob/main/DEVELOPER_GUIDE.md#openapi-extensions for field details.

Since the tools don't allow the same field name twice, I think we need to do something like this:

        total_indexing_buffer_in_bytes:
          oneOf:
            - $ref: '_common.yaml#/components/schemas/HumanReadableByteCount'
              x-version-removed: 3.0
            - $ref: '_common.yaml#/components/schemas/ByteCount'
              x-version-added: 3.0

You can run the merge (see https://github.com/opensearch-project/opensearch-api-specification/blob/main/DEVELOPER_GUIDE.md#spec-merger) with an option to target 3.0 and it should remove the oneOf that's not matching.

If that doesn't work, then let's do the above minus the x-version options with a comment.

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.

2 participants