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

[BUG] NodesInfoResponse serializes total_indexing_buffer and total_indexing_buffer_in_bytes swapped #16910

Closed
Xtansia opened this issue Dec 24, 2024 · 3 comments · Fixed by #17070
Labels
bug Something isn't working Cluster Manager v3.0.0 Issues and PRs related to version 3.0.0

Comments

@Xtansia
Copy link
Contributor

Xtansia commented Dec 24, 2024

Describe the bug

When retrieving node info total_indexing_buffer_in_bytes should be the raw byte count (e.g. 53687091) and total_indexing_buffer should be the human readable version (e.g. 51.1mb). This is currently swapped and is against the patterns set out by other human byte count properties.

This discrepancy was introduced >8 years ago: ebc3c17#diff-0f31418774699d0487aba335ddccd8942599b4a9999e92e2932bcfb6a22aa392
The two explicit fields were merged to use on of the "human readable field" XContentBuilder methods to enable use of the ?human parameter, however the parameters to byteSizeField were swapped.

Related component

Cluster Manager

To Reproduce

  1. Execute GET /_nodes?human&filter_path=nodes.*.total_indexing_buffer*
  2. See the response:
{
  "nodes": {
    "741KuezCTUaGy0RDV7oxEA": {
      "total_indexing_buffer_in_bytes": "51.1mb",
      "total_indexing_buffer": 53687091
    },
    "eTLqEyRiQzaxSYdZ1KbfVA": {
      "total_indexing_buffer_in_bytes": "51.1mb",
      "total_indexing_buffer": 53687091
    }
  }
}

Expected behavior

The total_indexing_buffer_in_bytes field should contain the raw byte count and the total_indexing_buffer field should contain the human readable version.

Additional Details

No response

@rajiv-kv
Copy link
Contributor

rajiv-kv commented Jan 6, 2025

[Triage - attendees 1 2 3]
Thanks @Xtansia for filing the issue. This seems to be different from the pattern used by stats API.
Please feel free to raise the fix.

@rajiv-kv rajiv-kv moved this from 🆕 New to Later (6 months plus) in Cluster Manager Project Board Jan 16, 2025
@rajiv-kv rajiv-kv moved this from Later (6 months plus) to 🆕 New in Cluster Manager Project Board Jan 16, 2025
@rajiv-kv rajiv-kv moved this from 🆕 New to Next (Next Quarter) in Cluster Manager Project Board Jan 16, 2025
@hye-on
Copy link
Contributor

hye-on commented Jan 17, 2025

Hi, @Xtansia

Can I work on this? I see a few options:

  1. Directly swap the field values:
    total_indexing_buffer <-> total_indexing_buffer_in_bytes

  2. Add new fields with different names for backward compatibility

  3. Handle it with version check:

Version currentVersion = Version.CURRENT;
if (currentVersion.onOrAfter(Version.V_3_0_0)) {
    builder.humanReadableField("total_indexing_buffer_in_bytes", "total_indexing_buffer", nodeInfo.getTotalIndexingBuffer());
} else {
    builder.humanReadableField("total_indexing_buffer", "total_indexing_buffer_in_bytes", nodeInfo.getTotalIndexingBuffer());
}

Thank you, and I look forward to your feedback :)

@Xtansia
Copy link
Contributor Author

Xtansia commented Jan 19, 2025

@hye-on Feel free to pick this up!
Swapping the fields will technically be a breaking change, so it will need to be either behind a version check or just not backported to 2.x and only merged into main, not sure on the general preference. I think adding new fields with different names is likely to add more confusion than it helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Cluster Manager v3.0.0 Issues and PRs related to version 3.0.0
Projects
Status: ✅ Done
4 participants