Skip to content

Commit 947a0ed

Browse files
committed
Handle -1 gc_threshold settings explicitly (elastic#54546)
Because -1 is technically a valid TimeValue (as a sentinel value), that is now explicitly checked for when validating gc_thresholds. The tests are also adjusted to test this case separately from other negative values.
1 parent 1a5d508 commit 947a0ed

File tree

2 files changed

+16
-1
lines changed

2 files changed

+16
-1
lines changed

server/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,10 @@ private static TimeValue getValidThreshold(Settings settings, String key, String
177177

178178
if (threshold == null) {
179179
throw new IllegalArgumentException("missing gc_threshold for [" + getThresholdName(key, level) + "]");
180+
} else if (threshold.nanos() < 0) {
181+
final String settingValue = settings.get(level);
182+
throw new IllegalArgumentException("invalid gc_threshold [" + getThresholdName(key, level) + "] value [" +
183+
settingValue + "]: value cannot be negative");
180184
}
181185

182186
return threshold;

server/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ public void testDisabledSetting() throws InterruptedException {
6262

6363
public void testNegativeSetting() throws InterruptedException {
6464
String collector = randomAlphaOfLength(5);
65-
final String timeValue = "-" + randomTimeValue();
65+
final String timeValue = "-" + randomTimeValue(2,1000); // -1 is handled separately
6666
Settings settings = Settings.builder().put("monitor.jvm.gc.collector." + collector + ".warn", timeValue).build();
6767
execute(settings, (command, interval, name) -> null, e -> {
6868
assertThat(e, instanceOf(IllegalArgumentException.class));
@@ -71,6 +71,17 @@ public void testNegativeSetting() throws InterruptedException {
7171
}, true, null);
7272
}
7373

74+
public void testNegativeOneSetting() throws InterruptedException {
75+
String collector = randomAlphaOfLength(5);
76+
final String timeValue = "-1" + randomFrom("", "d", "h", "m", "s", "ms", "nanos");
77+
Settings settings = Settings.builder().put("monitor.jvm.gc.collector." + collector + ".warn", timeValue).build();
78+
execute(settings, (command, interval, name) -> null, e -> {
79+
assertThat(e, instanceOf(IllegalArgumentException.class));
80+
assertThat(e.getMessage(), equalTo("invalid gc_threshold [monitor.jvm.gc.collector." + collector + ".warn] " +
81+
"value [" + timeValue + "]: value cannot be negative"));
82+
}, true, null);
83+
}
84+
7485
public void testMissingSetting() throws InterruptedException {
7586
String collector = randomAlphaOfLength(5);
7687
Set<AbstractMap.SimpleEntry<String, String>> entries = new HashSet<>();

0 commit comments

Comments
 (0)