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

Fix setting by time unit (#37192) 

This commit fixes an issue with a settings builder method that allows
setting a duration by time unit. In particular, this method can suffer
from a loss of precision. For example, if the input duration is 1500
microseconds then internally we are converting this to "1ms",
demonstrating the loss of precision. Instead, we should internally
convert this to a TimeValue that correctly represents the input
duration, and then convert this to a string using a method that does not
lose the unit. That is what this commit does.
This commit is contained in:
Jason Tedor 2019-01-07 14:59:24 -08:00 committed by GitHub
parent 382e4d39ef
commit 3b48b99861
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 18 additions and 3 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

@ -1019,8 +1019,8 @@ public final class Settings implements ToXContentFragment {
* @param value The time value
* @return The builder
*/
public Builder put(String setting, long value, TimeUnit timeUnit) {
put(setting, timeUnit.toMillis(value) + "ms");
public Builder put(final String setting, final long value, final TimeUnit timeUnit) {
put(setting, new TimeValue(value, timeUnit));
return this;
}

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

@ -47,6 +47,7 @@ import java.util.Iterator;
import java.util.Map;
import java.util.NoSuchElementException;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsInAnyOrder;
@ -744,4 +745,18 @@ public class SettingsTests extends ESTestCase {
assertThat(actual, equalTo(expected));
}
public void testSetByTimeUnit() {
final Setting<TimeValue> setting =
Setting.timeSetting("key", TimeValue.parseTimeValue(randomTimeValue(0, 24, "h"), "key"), TimeValue.ZERO);
final TimeValue expected = new TimeValue(1500, TimeUnit.MICROSECONDS);
final Settings settings = Settings.builder().put("key", expected.getMicros(), TimeUnit.MICROSECONDS).build();
/*
* Previously we would internally convert the duration to a string by converting to milliseconds which could lose precision (e.g.,
* 1500 microseconds would be converted to 1ms). Effectively this test is then asserting that we no longer make this mistake when
* doing the internal string conversion. Instead, we convert to a duration using a method that does not lose the original unit.
*/
final TimeValue actual = setting.get(settings);
assertThat(actual, equalTo(expected));
}
}

2
server/src/test/java/org/elasticsearch/snapshots/SharedClusterSnapshotRestoreIT.java
View File

@ -445,7 +445,7 @@ public class SharedClusterSnapshotRestoreIT extends AbstractSnapshotIntegTestCas
logger.info("--> assert that old settings are restored");
GetSettingsResponse getSettingsResponse = client.admin().indices().prepareGetSettings("test-idx").execute().actionGet();
assertThat(getSettingsResponse.getSetting("test-idx", "index.refresh_interval"), equalTo("10000ms"));
assertThat(getSettingsResponse.getSetting("test-idx", "index.refresh_interval"), equalTo("10s"));
}
public void testEmptySnapshot() throws Exception {