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.
This commit is contained in:
parent
c21057b3a2
commit
c95e7539e7
|
@ -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());
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
@ -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());
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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 {
|
||||
|
|
|
@ -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<T> 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;
|
||||
}
|
||||
|
|
|
@ -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<ByteSizeValue> byteSizeValueSetting =
|
||||
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);
|
||||
|
||||
|
||||
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 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);
|
||||
|
||||
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() {
|
||||
|
|
Loading…
Reference in New Issue