honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix handling of fractional time value settings (#37171) 

This commit addresses an issue when setting a time value setting using a
value that has a fractional component when converted to its string
representation. For example, trying to set a time value setting to a
value of 1500ms is problematic because internally this is converted to
the string "1.5s". When we go to get this setting, we try to parse
"1.5s" back to a time 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 time value to the string and back to the time value.
This commit is contained in:
Jason Tedor 2019-01-06 22:34:52 -08:00 committed by GitHub
parent da3d8fb5b7
commit bf5bc88f50
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 20 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
server/src/main/java/org/elasticsearch/common/settings/Settings.java
View File

@ -850,8 +850,8 @@ public final class Settings implements ToXContentFragment {
* @param timeValue The setting timeValue
* @return The builder
*/
public Builder put(String key, TimeValue timeValue) {
return put(key, timeValue.toString());
public Builder put(final String key, final TimeValue timeValue) {
return put(key, timeValue.getStringRep());
}
/**

18
server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java
View File

@ -25,6 +25,7 @@ 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.TimeValue;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
@ -708,4 +709,21 @@ public class SettingsTests extends ESTestCase {
IllegalArgumentException iae = expectThrows(IllegalArgumentException.class, () -> Settings.builder().copy("not_there", settings));
assertEquals("source key not found in the source settings", iae.getMessage());
}
public void testFractionalTimeValue() {
final Setting<TimeValue> setting =
Setting.timeSetting("key", TimeValue.parseTimeValue(randomTimeValue(0, 24, "h"), "key"), TimeValue.ZERO);
final TimeValue expected = TimeValue.timeValueMillis(randomNonNegativeLong());
final Settings settings = Settings.builder().put("key", expected).build();
/*
* Previously we would internally convert the time value to a string using a method that tries to be smart about the units (e.g.,
* 1000ms would be converted to 1s). However, this had a problem in that, for example, 1500ms would be converted to 1.5s. Then,
* 1.5s could not be converted back to a TimeValue because TimeValues 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 TimeValue actual = setting.get(settings);
assertThat(actual, equalTo(expected));
}
}