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] Fix Setting#byteSizeSetting to use whole value string #9011

Merged
merged 2 commits into from
Jul 31, 2023

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented Jul 31, 2023

This fixes a bug where the Setting#byteSizeSetting static method returned fractional byte values when calling .toString inside the lambda. The change is to directly return the bytes value as a string instead of using ByteSizeValue#toString which tries to be smart and pretty print in a more human-friendly format.

closes #9010
relates #9005

This fixes a bug where the Setting#byteSizeSetting static method
returned fractional byte values when calling .toString inside the
lambda. The change is to directly return the bytes value as a string
instead of using ByteSizeValue#toString which tries to be smart and
by pretty printing in a more human-friendly format.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize
Copy link
Collaborator Author

nknize commented Jul 31, 2023

This is currently blocking passing builds on main.

@nknize nknize changed the title [Bug] Fix Setting#byteSizeSetting to return whole value string [Bug] Fix Setting#byteSizeSetting to use whole value string Jul 31, 2023
@nknize
Copy link
Collaborator Author

nknize commented Jul 31, 2023

@reta mind giving this a quick once over and approval so we can unblock test failures?

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.repositories.azure.AzureBlobContainerRetriesTests.testWriteLargeBlob
      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=cluster.allocation_explain/10_basic/cluster shard allocation explanation test with empty request}

@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #9011 (994479b) into main (35662f0) will increase coverage by 0.03%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #9011      +/-   ##
============================================
+ Coverage     71.02%   71.05%   +0.03%     
- Complexity    57210    57268      +58     
============================================
  Files          4764     4764              
  Lines        270156   270153       -3     
  Branches      39532    39532              
============================================
+ Hits         191887   191967      +80     
+ Misses        62100    62056      -44     
+ Partials      16169    16130      -39     
Files Changed Coverage Δ
...n/java/org/opensearch/common/settings/Setting.java 85.87% <100.00%> (ø)
...java/org/opensearch/common/unit/ByteSizeValue.java 99.08% <100.00%> (+0.86%) ⬆️

... and 442 files with indirect coverage changes

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=cluster.allocation_explain/10_basic/cluster shard allocation explanation test with empty request}

@nknize
Copy link
Collaborator Author

nknize commented Jul 31, 2023

I'm going to go ahead and YOLO merge this to unblock the failures on main.

@nknize nknize merged commit dea6889 into opensearch-project:main Jul 31, 2023
14 checks passed
@dbwiddis
Copy link
Member

@nknize was about to say LGTM except for change log.

Can you add a change log entry in your next commit?

@dbwiddis
Copy link
Member

Or I see skip-changelog was specifically added.... okay...

@nknize
Copy link
Collaborator Author

nknize commented Jul 31, 2023

Or I see skip-changelog was specifically added.... okay...

Yeah.. This changelog mechanism is pretty broken. 😕 I can go back and add an entry for this if you think it's really needed.

kaushalmahi12 pushed a commit to kaushalmahi12/OpenSearch that referenced this pull request Sep 12, 2023
…ch-project#9011)

This fixes a bug where the Setting#byteSizeSetting static method
returned fractional byte values when calling .toString inside the
lambda. The change is to directly return the bytes value as a string
instead of using ByteSizeValue#toString which tries to be smart and
by pretty printing in a more human-friendly format.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>
brusic pushed a commit to brusic/OpenSearch that referenced this pull request Sep 25, 2023
…ch-project#9011)

This fixes a bug where the Setting#byteSizeSetting static method
returned fractional byte values when calling .toString inside the
lambda. The change is to directly return the bytes value as a string
instead of using ByteSizeValue#toString which tries to be smart and
by pretty printing in a more human-friendly format.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Ivan Brusic <ivan.brusic@flocksafety.com>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…ch-project#9011)

This fixes a bug where the Setting#byteSizeSetting static method
returned fractional byte values when calling .toString inside the
lambda. The change is to directly return the bytes value as a string
instead of using ByteSizeValue#toString which tries to be smart and
by pretty printing in a more human-friendly format.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Setting#byteSizeSetting returns a fractional byte size string for the default value
2 participants