From a233db7367844e4c91fc671a9dbb2bc0f8ba8dfe Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 7 Jan 2019 07:13:50 -0800 Subject: [PATCH] Fix handling of fractional byte size value settings (#37172) This commit addresses an issue when setting a byte size value setting using a value that has a fractional component when converted to its string representation. For example, trying to set a byte size value setting to a value of 1536 bytes is problematic because internally this is converted to the string "1.5k". When we go to get this setting, we try to parse "1.5k" back to a byte size value, which does not support fractional values. The problem is that internally we are relying on a method which loses the unit when doing the string conversion. Instead, we are going to use a method that does not lose the unit and therefore we can roundtrip from the byte size value to the string and back to the byte size value. --- .../common/settings/Settings.java | 4 ++-- .../common/settings/SettingsTests.java | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/Settings.java b/server/src/main/java/org/elasticsearch/common/settings/Settings.java index 30a95860e73..e8ba6d383d5 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -861,8 +861,8 @@ public final class Settings implements ToXContentFragment { * @param byteSizeValue The setting value * @return The builder */ - public Builder put(String key, ByteSizeValue byteSizeValue) { - return put(key, byteSizeValue.toString()); + public Builder put(final String key, final ByteSizeValue byteSizeValue) { + return put(key, byteSizeValue.getStringRep()); } /** diff --git a/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java b/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java index b48293a858d..27a9b002042 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java @@ -25,6 +25,8 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.unit.ByteSizeUnit; +import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -726,4 +728,20 @@ public class SettingsTests extends ESTestCase { assertThat(actual, equalTo(expected)); } + public void testFractionalByteSizeValue() { + final Setting setting = + Setting.byteSizeSetting("key", ByteSizeValue.parseBytesSizeValue(randomIntBetween(1, 16) + "k", "key")); + final ByteSizeValue expected = new ByteSizeValue(randomNonNegativeLong(), ByteSizeUnit.BYTES); + final Settings settings = Settings.builder().put("key", expected).build(); + /* + * Previously we would internally convert the byte size value to a string using a method that tries to be smart about the units + * (e.g., 1024 bytes would be converted to 1kb). However, this had a problem in that, for example, 1536 bytes would be converted to + * 1.5k. Then, 1.5k could not be converted back to a ByteSizeValue because ByteSizeValues do not support fractional components. + * Effectively this test is then asserting that we no longer make this mistake when doing the internal string conversion. Instead, + * we convert to a string using a method that does not lose the original unit. + */ + final ByteSizeValue actual = setting.get(settings); + assertThat(actual, equalTo(expected)); + } + }