From a1fd01d4ccb91a3a10d7d19446da13010413cb86 Mon Sep 17 00:00:00 2001
From: Jason Tedor <jason@tedor.me>
Date: Fri, 29 Jan 2016 09:39:02 -0500
Subject: [PATCH] No leniency in parsing JVM GC monitor settings

This commit removes some leniency in the parsing of the JVM GC monitor
settings, and adds tests for the JVM GC monitor settings.
---
 .../monitor/jvm/JvmGcMonitorService.java      |  31 +++--
 .../jvm/JvmGcMonitorServiceSettingsTests.java | 113 ++++++++++++++++++
 2 files changed, 135 insertions(+), 9 deletions(-)
 create mode 100644 core/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java

diff --git a/core/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java b/core/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java
index 1d0da9821b5..97c813a0fe3 100644
--- a/core/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java
+++ b/core/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java
@@ -50,7 +50,9 @@ public class JvmGcMonitorService extends AbstractLifecycleComponent<JvmGcMonitor
     public final static Setting<Boolean> ENABLED_SETTING = Setting.boolSetting("monitor.jvm.gc.enabled", true, false, Scope.CLUSTER);
     public final static Setting<TimeValue> REFRESH_INTERVAL_SETTING =
         Setting.timeSetting("monitor.jvm.gc.refresh_interval", TimeValue.timeValueSeconds(1), TimeValue.timeValueSeconds(1), false, Scope.CLUSTER);
-    public final static Setting<Settings> GC_SETTING = Setting.groupSetting("monitor.jvm.gc.collector.", false, Scope.CLUSTER);
+
+    private static String GC_COLLECTOR_PREFIX = "monitor.jvm.gc.collector.";
+    public final static Setting<Settings> GC_SETTING = Setting.groupSetting(GC_COLLECTOR_PREFIX, false, Scope.CLUSTER);
 
     static class GcThreshold {
         public final String name;
@@ -87,14 +89,10 @@ public class JvmGcMonitorService extends AbstractLifecycleComponent<JvmGcMonitor
         Map<String, Settings> gcThresholdGroups = GC_SETTING.get(settings).getAsGroups();
         for (Map.Entry<String, Settings> entry : gcThresholdGroups.entrySet()) {
             String name = entry.getKey();
-            TimeValue warn = entry.getValue().getAsTime("warn", null);
-            TimeValue info = entry.getValue().getAsTime("info", null);
-            TimeValue debug = entry.getValue().getAsTime("debug", null);
-            if (warn == null || info == null || debug == null) {
-                logger.warn("ignoring gc_threshold for [{}], missing warn/info/debug values", name);
-            } else {
-                gcThresholds.put(name, new GcThreshold(name, warn.millis(), info.millis(), debug.millis()));
-            }
+            TimeValue warn = getValidThreshold(entry.getValue(), entry.getKey(), "warn");
+            TimeValue info = getValidThreshold(entry.getValue(), entry.getKey(), "info");
+            TimeValue debug = getValidThreshold(entry.getValue(), entry.getKey(), "debug");
+            gcThresholds.put(name, new GcThreshold(name, warn.millis(), info.millis(), debug.millis()));
         }
         gcThresholds.putIfAbsent(GcNames.YOUNG, new GcThreshold(GcNames.YOUNG, 1000, 700, 400));
         gcThresholds.putIfAbsent(GcNames.OLD, new GcThreshold(GcNames.OLD, 10000, 5000, 2000));
@@ -104,6 +102,21 @@ public class JvmGcMonitorService extends AbstractLifecycleComponent<JvmGcMonitor
         logger.debug("enabled [{}], interval [{}], gc_threshold [{}]", enabled, interval, this.gcThresholds);
     }
 
+    private static TimeValue getValidThreshold(Settings settings, String key, String level) {
+        TimeValue threshold = settings.getAsTime(level, null);
+        if (threshold == null) {
+            throw new IllegalArgumentException("missing gc_threshold for [" + getThresholdName(key, level) + "]");
+        }
+        if (threshold.nanos() <= 0) {
+            throw new IllegalArgumentException("invalid gc_threshold [" + threshold + "] for [" + getThresholdName(key, level) + "]");
+        }
+        return threshold;
+    }
+
+    private static String getThresholdName(String key, String level) {
+        return GC_COLLECTOR_PREFIX + key + "." + level;
+    }
+
     @Override
     protected void doStart() {
         if (!enabled) {
diff --git a/core/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java b/core/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java
new file mode 100644
index 00000000000..29f497458c7
--- /dev/null
+++ b/core/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java
@@ -0,0 +1,113 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.monitor.jvm;
+
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.unit.TimeValue;
+import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.threadpool.ThreadPool;
+
+import java.util.AbstractMap;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.BiFunction;
+import java.util.function.Consumer;
+
+import static org.hamcrest.CoreMatchers.allOf;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.instanceOf;
+
+public class JvmGcMonitorServiceSettingsTests extends ESTestCase {
+
+    public void testEmptySettingsAreOkay() throws InterruptedException {
+        AtomicBoolean scheduled = new AtomicBoolean();
+        execute(Settings.EMPTY, (command, interval) -> { scheduled.set(true); return null; }, () -> assertTrue(scheduled.get()));
+    }
+
+    public void testDisabledSetting() throws InterruptedException {
+        Settings settings = Settings.builder().put("monitor.jvm.gc.enabled", "false").build();
+        AtomicBoolean scheduled = new AtomicBoolean();
+        execute(settings, (command, interval) -> { scheduled.set(true); return null; }, () -> assertFalse(scheduled.get()));
+    }
+
+    public void testNegativeSetting() throws InterruptedException {
+        String collector = randomAsciiOfLength(5);
+        Settings settings = Settings.builder().put("monitor.jvm.gc.collector." + collector + ".warn", "-" + randomTimeValue()).build();
+        execute(settings, (command, interval) -> null, t -> {
+            assertThat(t, instanceOf(IllegalArgumentException.class));
+            assertThat(t.getMessage(), allOf(containsString("invalid gc_threshold"), containsString("for [monitor.jvm.gc.collector." + collector + ".")));
+        }, true, null);
+    }
+
+    public void testMissingSetting() throws InterruptedException {
+        String collector = randomAsciiOfLength(5);
+        Set<AbstractMap.SimpleEntry<String, String>> entries = new HashSet<>();
+        entries.add(new AbstractMap.SimpleEntry<>("monitor.jvm.gc.collector." + collector + ".warn", randomTimeValue()));
+        entries.add(new AbstractMap.SimpleEntry<>("monitor.jvm.gc.collector." + collector + ".info", randomTimeValue()));
+        entries.add(new AbstractMap.SimpleEntry<>("monitor.jvm.gc.collector." + collector + ".debug", randomTimeValue()));
+        Settings.Builder builder = Settings.builder();
+
+        // drop a random setting or two
+        for (@SuppressWarnings("unchecked") AbstractMap.SimpleEntry<String, String> entry : randomSubsetOf(randomIntBetween(1, 2), entries.toArray(new AbstractMap.SimpleEntry[0]))) {
+            builder.put(entry.getKey(), entry.getValue());
+        }
+
+        // we should get an exception that a setting is missing
+        execute(builder.build(), (command, interval) -> null, t -> {
+            assertThat(t, instanceOf(IllegalArgumentException.class));
+            assertThat(t.getMessage(), containsString("missing gc_threshold for [monitor.jvm.gc.collector." + collector + "."));
+        }, true, null);
+    }
+
+    private static void execute(Settings settings, BiFunction<Runnable, TimeValue, ScheduledFuture<?>> scheduler, Runnable asserts) throws InterruptedException {
+        execute(settings, scheduler, null, false, asserts);
+    }
+
+    private static void execute(Settings settings, BiFunction<Runnable, TimeValue, ScheduledFuture<?>> scheduler, Consumer<Throwable> consumer, boolean constructionShouldFail, Runnable asserts) throws InterruptedException {
+        assert constructionShouldFail == (consumer != null);
+        assert constructionShouldFail == (asserts == null);
+        ThreadPool threadPool = null;
+        try {
+            threadPool = new ThreadPool(JvmGcMonitorServiceSettingsTests.class.getCanonicalName()) {
+                @Override
+                public ScheduledFuture<?> scheduleWithFixedDelay(Runnable command, TimeValue interval) {
+                    return scheduler.apply(command, interval);
+                }
+            };
+            try {
+                JvmGcMonitorService service = new JvmGcMonitorService(settings, threadPool);
+                if (constructionShouldFail) {
+                    fail("construction of jvm gc service should have failed");
+                }
+                service.doStart();
+                asserts.run();
+                service.doStop();
+            } catch (Throwable t) {
+                consumer.accept(t);
+            }
+        } finally {
+            ThreadPool.terminate(threadPool, 30, TimeUnit.SECONDS);
+        }
+    }
+
+}