From 3b48b9986110a3dbb89d3dbf5ad35f9e8b98ef00 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 e8ba6d383d55e..ac43a1800b40f 100644
--- a/server/src/main/java/org/elasticsearch/common/settings/Settings.java
+++ b/server/src/main/java/org/elasticsearch/common/settings/Settings.java
@@ -1019,8 +1019,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 27a9b00204203..802bceaa90812 100644
--- a/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java
+++ b/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java
@@ -47,6 +47,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;
@@ -744,4 +745,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 d7ca90a90a3d9..c90d9319df30b 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 {