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 e8ba6d383d5..ac43a1800b4 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 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;
         }
 
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 27a9b002042..802bceaa908 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.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));
+    }
+
 }
diff --git a/server/src/test/java/org/elasticsearch/snapshots/SharedClusterSnapshotRestoreIT.java b/server/src/test/java/org/elasticsearch/snapshots/SharedClusterSnapshotRestoreIT.java
index d7ca90a90a3..c90d9319df3 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 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 {