From d290a0a6be6ec2bf836be9ae2cfed7e63c862445 Mon Sep 17 00:00:00 2001
From: Jason Tedor <jason@tedor.me>
Date: Mon, 7 Jan 2019 14:59:24 -0800
Subject: [PATCH] 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.
---
 .../elasticsearch/common/settings/Settings.java   |  4 ++--
 .../common/settings/SettingsTests.java            | 15 +++++++++++++++
 .../snapshots/SharedClusterSnapshotRestoreIT.java |  2 +-
 3 files changed, 18 insertions(+), 3 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 1a6dca31e8b3c..26b15e3ac2321 100644
--- a/server/src/main/java/org/elasticsearch/common/settings/Settings.java
+++ b/server/src/main/java/org/elasticsearch/common/settings/Settings.java
@@ -1061,8 +1061,8 @@ public Builder put(String setting, double value) {
          * @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;
         }
 
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 7f0b19db97209..1607f03188d78 100644
--- a/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java
+++ b/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java
@@ -54,6 +54,7 @@
 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;
@@ -856,4 +857,18 @@ public void testFractionalByteSizeValue() {
         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));
+    }
+
 }
diff --git a/server/src/test/java/org/elasticsearch/snapshots/SharedClusterSnapshotRestoreIT.java b/server/src/test/java/org/elasticsearch/snapshots/SharedClusterSnapshotRestoreIT.java
index eec2aca3a0722..43acbb2338a17 100644
--- a/server/src/test/java/org/elasticsearch/snapshots/SharedClusterSnapshotRestoreIT.java
+++ b/server/src/test/java/org/elasticsearch/snapshots/SharedClusterSnapshotRestoreIT.java
@@ -445,7 +445,7 @@ public void testRestoreWithDifferentMappingsAndSettings() throws Exception {
 
         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 {