Skip to content

Commit

Permalink
Fix handling of fractional byte size value settings (#37172)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jasontedor committed Jan 7, 2019
1 parent 7fabc1b commit 9b9b6d9
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -903,8 +903,8 @@ public Builder put(final String key, final TimeValue timeValue) {
* @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());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import org.elasticsearch.common.io.stream.BytesStreamOutput;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.logging.DeprecationLogger;
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.XContent;
Expand Down Expand Up @@ -838,4 +840,20 @@ public void testFractionalTimeValue() {
assertThat(actual, equalTo(expected));
}

public void testFractionalByteSizeValue() {
final Setting<ByteSizeValue> 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));
}

}

0 comments on commit 9b9b6d9

Please sign in to comment.