Enhance error for out of bounds byte size settings (#29338)

Today when you input a byte size setting that is out of bounds for the
setting, you get an error message that indicates the maximum value of
the setting. The problem is that because we use ByteSize#toString, we
end up with a representation of the value that does not really tell you
what the bound is. For example, if the bound is 2^31 - 1 bytes, the
output would be 1.9gb which does not really tell you want the limit as
there are many byte size values that we format to the same 1.9gb with
ByteSize#toString. We have a method ByteSize#getStringRep that uses the
input units to the value as the output units for the string
representation, so we end up with no loss if we use this to report the
bound. This commit does this.

Author: jasontedor

plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureRepositorySettingsTests.java

@@ -112,17 +112,17 @@ public void testChunkSize() throws StorageException, IOException, URISyntaxExcep
         // zero bytes is not allowed
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
             azureRepository(Settings.builder().put("chunk_size", "0").build()));
-        assertEquals("Failed to parse value [0] for setting [chunk_size] must be >= 1b", e.getMessage());
+        assertEquals("failed to parse value [0] for setting [chunk_size], must be >= [1b]", e.getMessage());
 
         // negative bytes not allowed
         e = expectThrows(IllegalArgumentException.class, () ->
             azureRepository(Settings.builder().put("chunk_size", "-1").build()));
-        assertEquals("Failed to parse value [-1] for setting [chunk_size] must be >= 1b", e.getMessage());
+        assertEquals("failed to parse value [-1] for setting [chunk_size], must be >= [1b]", e.getMessage());
 
         // greater than max chunk size not allowed
         e = expectThrows(IllegalArgumentException.class, () ->
             azureRepository(Settings.builder().put("chunk_size", "65mb").build()));
-        assertEquals("Failed to parse value [65mb] for setting [chunk_size] must be <= 64mb", e.getMessage());
+        assertEquals("failed to parse value [65mb] for setting [chunk_size], must be <= [64mb]", e.getMessage());
     }
 
 }

plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java

@@ -110,22 +110,22 @@ public void testChunkSize() {
                                                                         Settings.builder().put("chunk_size", "0").build());
             GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetaData);
         });
-        assertEquals("Failed to parse value [0] for setting [chunk_size] must be >= 1b", e.getMessage());
+        assertEquals("failed to parse value [0] for setting [chunk_size], must be >= [1b]", e.getMessage());
 
         // negative bytes not allowed
         e = expectThrows(IllegalArgumentException.class, () -> {
             RepositoryMetaData repoMetaData = new RepositoryMetaData("repo", GoogleCloudStorageRepository.TYPE,
                                                                         Settings.builder().put("chunk_size", "-1").build());
             GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetaData);
         });
-        assertEquals("Failed to parse value [-1] for setting [chunk_size] must be >= 1b", e.getMessage());
+        assertEquals("failed to parse value [-1] for setting [chunk_size], must be >= [1b]", e.getMessage());
 
         // greater than max chunk size not allowed
         e = expectThrows(IllegalArgumentException.class, () -> {
             RepositoryMetaData repoMetaData = new RepositoryMetaData("repo", GoogleCloudStorageRepository.TYPE,
                                                                         Settings.builder().put("chunk_size", "101mb").build());
             GoogleCloudStorageRepository.getSetting(GoogleCloudStorageRepository.CHUNK_SIZE, repoMetaData);
         });
-        assertEquals("Failed to parse value [101mb] for setting [chunk_size] must be <= 100mb", e.getMessage());
+        assertEquals("failed to parse value [101mb] for setting [chunk_size], must be <= [100mb]", e.getMessage());
     }
 }

plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java

@@ -70,10 +70,10 @@ public void testInvalidChunkBufferSizeSettings() throws IOException {
         assertValidBuffer(5, 5);
         // buffer < 5mb should fail
         assertInvalidBuffer(4, 10, IllegalArgumentException.class,
-                "Failed to parse value [4mb] for setting [buffer_size] must be >= 5mb");
+                "failed to parse value [4mb] for setting [buffer_size], must be >= [5mb]");
         // chunk > 5tb should fail
         assertInvalidBuffer(5, 6000000, IllegalArgumentException.class,
-                "Failed to parse value [6000000mb] for setting [chunk_size] must be <= 5tb");
+                "failed to parse value [6000000mb] for setting [chunk_size], must be <= [5tb]");
     }
 
     private void assertValidBuffer(long bufferMB, long chunkMB) throws IOException {

server/src/main/java/org/elasticsearch/common/settings/Setting.java

@@ -47,6 +47,7 @@
 import java.util.IdentityHashMap;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
@@ -1070,10 +1071,22 @@ public static Setting<ByteSizeValue> byteSizeSetting(String key, Function<Settin
     public static ByteSizeValue parseByteSize(String s, ByteSizeValue minValue, ByteSizeValue maxValue, String key) {
         ByteSizeValue value = ByteSizeValue.parseBytesSizeValue(s, key);
         if (value.getBytes() < minValue.getBytes()) {
-            throw new IllegalArgumentException("Failed to parse value [" + s + "] for setting [" + key + "] must be >= " + minValue);
+            final String message = String.format(
+                    Locale.ROOT,
+                    "failed to parse value [%s] for setting [%s], must be >= [%s]",
+                    s,
+                    key,
+                    minValue.getStringRep());
+            throw new IllegalArgumentException(message);
         }
         if (value.getBytes() > maxValue.getBytes()) {
-            throw new IllegalArgumentException("Failed to parse value [" + s + "] for setting [" + key + "] must be <= " + maxValue);
+            final String message = String.format(
+                    Locale.ROOT,
+                    "failed to parse value [%s] for setting [%s], must be <= [%s]",
+                    s,
+                    key,
+                    maxValue.getStringRep());
+            throw new IllegalArgumentException(message);
         }
         return value;
     }

server/src/test/java/org/elasticsearch/common/settings/SettingTests.java

@@ -20,6 +20,7 @@
 
 import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.settings.Setting.Property;
+import org.elasticsearch.common.unit.ByteSizeUnit;
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.monitor.jvm.JvmInfo;
@@ -52,35 +53,61 @@ public void testGet() {
         assertTrue(booleanSetting.get(Settings.builder().put("foo.bar", true).build()));
     }
 
-    public void testByteSize() {
-        Setting<ByteSizeValue> byteSizeValueSetting =
-            Setting.byteSizeSetting("a.byte.size", new ByteSizeValue(1024), Property.Dynamic, Property.NodeScope);
+    public void testByteSizeSetting() {
+        final Setting<ByteSizeValue> byteSizeValueSetting =
+                Setting.byteSizeSetting("a.byte.size", new ByteSizeValue(1024), Property.Dynamic, Property.NodeScope);
         assertFalse(byteSizeValueSetting.isGroupSetting());
-        ByteSizeValue byteSizeValue = byteSizeValueSetting.get(Settings.EMPTY);
-        assertEquals(byteSizeValue.getBytes(), 1024);
-
-        byteSizeValueSetting = Setting.byteSizeSetting("a.byte.size", s -> "2048b", Property.Dynamic, Property.NodeScope);
-        byteSizeValue = byteSizeValueSetting.get(Settings.EMPTY);
-        assertEquals(byteSizeValue.getBytes(), 2048);
-
-
+        final ByteSizeValue byteSizeValue = byteSizeValueSetting.get(Settings.EMPTY);
+        assertThat(byteSizeValue.getBytes(), equalTo(1024L));
+    }
+
+    public void testByteSizeSettingMinValue() {
+        final Setting<ByteSizeValue> byteSizeValueSetting =
+                Setting.byteSizeSetting(
+                        "a.byte.size",
+                        new ByteSizeValue(100, ByteSizeUnit.MB),
+                        new ByteSizeValue(20_000_000, ByteSizeUnit.BYTES),
+                        new ByteSizeValue(Integer.MAX_VALUE, ByteSizeUnit.BYTES));
+        final long value = 20_000_000 - randomIntBetween(1, 1024);
+        final Settings settings = Settings.builder().put("a.byte.size", value + "b").build();
+        final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> byteSizeValueSetting.get(settings));
+        final String expectedMessage = "failed to parse value [" + value + "b] for setting [a.byte.size], must be >= [20000000b]";
+        assertThat(e, hasToString(containsString(expectedMessage)));
+    }
+
+    public void testByteSizeSettingMaxValue() {
+        final Setting<ByteSizeValue> byteSizeValueSetting =
+                Setting.byteSizeSetting(
+                        "a.byte.size",
+                        new ByteSizeValue(100, ByteSizeUnit.MB),
+                        new ByteSizeValue(16, ByteSizeUnit.MB),
+                        new ByteSizeValue(Integer.MAX_VALUE, ByteSizeUnit.BYTES));
+        final long value = (1L << 31) - 1 + randomIntBetween(1, 1024);
+        final Settings settings = Settings.builder().put("a.byte.size", value + "b").build();
+        final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> byteSizeValueSetting.get(settings));
+        final String expectedMessage = "failed to parse value [" + value + "b] for setting [a.byte.size], must be <= [2147483647b]";
+        assertThat(e, hasToString(containsString(expectedMessage)));
+    }
+
+    public void testByteSizeSettingValidation() {
+        final Setting<ByteSizeValue> byteSizeValueSetting =
+                Setting.byteSizeSetting("a.byte.size", s -> "2048b", Property.Dynamic, Property.NodeScope);
+        final ByteSizeValue byteSizeValue = byteSizeValueSetting.get(Settings.EMPTY);
+        assertThat(byteSizeValue.getBytes(), equalTo(2048L));
         AtomicReference<ByteSizeValue> value = new AtomicReference<>(null);
         ClusterSettings.SettingUpdater<ByteSizeValue> settingUpdater = byteSizeValueSetting.newUpdater(value::set, logger);
-        try {
-            settingUpdater.apply(Settings.builder().put("a.byte.size", 12).build(), Settings.EMPTY);
-            fail("no unit");
-        } catch (IllegalArgumentException ex) {
-            assertThat(ex, hasToString(containsString("illegal value can't update [a.byte.size] from [2048b] to [12]")));
-            assertNotNull(ex.getCause());
-            assertThat(ex.getCause(), instanceOf(IllegalArgumentException.class));
-            final IllegalArgumentException cause = (IllegalArgumentException) ex.getCause();
-            final String expected =
-                    "failed to parse setting [a.byte.size] with value [12] as a size in bytes: unit is missing or unrecognized";
-            assertThat(cause, hasToString(containsString(expected)));
-        }
 
+        final IllegalArgumentException e = expectThrows(
+                IllegalArgumentException.class,
+                () -> settingUpdater.apply(Settings.builder().put("a.byte.size", 12).build(), Settings.EMPTY));
+        assertThat(e, hasToString(containsString("illegal value can't update [a.byte.size] from [2048b] to [12]")));
+        assertNotNull(e.getCause());
+        assertThat(e.getCause(), instanceOf(IllegalArgumentException.class));
+        final IllegalArgumentException cause = (IllegalArgumentException) e.getCause();
+        final String expected = "failed to parse setting [a.byte.size] with value [12] as a size in bytes: unit is missing or unrecognized";
+        assertThat(cause, hasToString(containsString(expected)));
         assertTrue(settingUpdater.apply(Settings.builder().put("a.byte.size", "12b").build(), Settings.EMPTY));
-        assertEquals(new ByteSizeValue(12), value.get());
+        assertThat(value.get(), equalTo(new ByteSizeValue(12)));
     }
 
     public void testMemorySize() {