From c95e7539e7c090eba45b8a79c68570907d0b9e4a Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 4 Apr 2018 07:22:13 -0400 Subject: [PATCH] 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. --- .../azure/AzureRepositorySettingsTests.java | 6 +- ...eCloudStorageBlobStoreRepositoryTests.java | 6 +- .../repositories/s3/S3RepositoryTests.java | 4 +- .../common/settings/Setting.java | 17 ++++- .../common/settings/SettingTests.java | 69 +++++++++++++------ 5 files changed, 71 insertions(+), 31 deletions(-) diff --git a/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureRepositorySettingsTests.java b/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureRepositorySettingsTests.java index 75025332889..01b26bad343 100644 --- a/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureRepositorySettingsTests.java +++ b/plugins/repository-azure/src/test/java/org/elasticsearch/repositories/azure/AzureRepositorySettingsTests.java @@ -112,17 +112,17 @@ public class AzureRepositorySettingsTests extends ESTestCase { // 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()); } } diff --git a/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java index ec166ff867f..1a173b44065 100644 --- a/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java +++ b/plugins/repository-gcs/src/test/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java @@ -110,7 +110,7 @@ public class GoogleCloudStorageBlobStoreRepositoryTests extends ESBlobStoreRepos 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, () -> { @@ -118,7 +118,7 @@ public class GoogleCloudStorageBlobStoreRepositoryTests extends ESBlobStoreRepos 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, () -> { @@ -126,6 +126,6 @@ public class GoogleCloudStorageBlobStoreRepositoryTests extends ESBlobStoreRepos 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()); } } diff --git a/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java b/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java index 93508f11c09..7da65c27d81 100644 --- a/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java +++ b/plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java @@ -70,10 +70,10 @@ public class S3RepositoryTests extends ESTestCase { 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 { diff --git a/server/src/main/java/org/elasticsearch/common/settings/Setting.java b/server/src/main/java/org/elasticsearch/common/settings/Setting.java index 9575862194d..9d4ee53aa1a 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -47,6 +47,7 @@ import java.util.HashSet; 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 class Setting implements ToXContentObject { 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; } diff --git a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java index 180f11730df..187c0e21b4d 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java @@ -20,6 +20,7 @@ package org.elasticsearch.common.settings; 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 class SettingTests extends ESTestCase { assertTrue(booleanSetting.get(Settings.builder().put("foo.bar", true).build())); } - public void testByteSize() { - Setting byteSizeValueSetting = - Setting.byteSizeSetting("a.byte.size", new ByteSizeValue(1024), Property.Dynamic, Property.NodeScope); + public void testByteSizeSetting() { + final Setting 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); + final ByteSizeValue byteSizeValue = byteSizeValueSetting.get(Settings.EMPTY); + assertThat(byteSizeValue.getBytes(), equalTo(1024L)); + } - byteSizeValueSetting = Setting.byteSizeSetting("a.byte.size", s -> "2048b", Property.Dynamic, Property.NodeScope); - byteSizeValue = byteSizeValueSetting.get(Settings.EMPTY); - assertEquals(byteSizeValue.getBytes(), 2048); + public void testByteSizeSettingMinValue() { + final Setting 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 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 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 value = new AtomicReference<>(null); ClusterSettings.SettingUpdater 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() {