Skip to content

Commit

Permalink
Enhance error for out of bounds byte size settings (#29338)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jasontedor committed Apr 4, 2018
1 parent 9b67011 commit 243df25
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 34 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -113,17 +113,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());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -114,22 +114,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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down

0 comments on commit 243df25

Please sign in to comment.