From 7d703c2f92ba784de47ea00959ba7626597e4e94 Mon Sep 17 00:00:00 2001
From: Simon Willnauer <simonw@apache.org>
Date: Mon, 24 Sep 2018 07:45:50 +0200
Subject: [PATCH] Fix AutoQueueAdjustingExecutorBuilder settings validation
 (#33922)

Settings validation in AutoQueueAdjustingExecutorBuilder always checked against
a default value which means that we never can change a max queue size that is lower
than the default. This change adds tests and fixes this validation.
---
 .../AutoQueueAdjustingExecutorBuilder.java    | 54 ++++++++++----
 ...utoQueueAdjustingExecutorBuilderTests.java | 70 ++++++++++++++++++-
 2 files changed, 109 insertions(+), 15 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java b/server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java
index ec9d95c722d..45f53006ecd 100644
--- a/server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java
+++ b/server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java
@@ -28,8 +28,10 @@ import org.elasticsearch.common.util.concurrent.ThreadContext;
 import org.elasticsearch.node.Node;
 
 import java.util.Arrays;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.ThreadFactory;
 
@@ -71,14 +73,42 @@ public final class AutoQueueAdjustingExecutorBuilder extends ExecutorBuilder<Aut
         Setting<Integer> tempMinQueueSizeSetting = Setting.intSetting(minSizeKey, minQueueSize, Setting.Property.NodeScope);
 
         this.minQueueSizeSetting = new Setting<>(
-                minSizeKey,
-                (s) -> Integer.toString(minQueueSize),
-                (s) -> Setting.parseInt(s, 0, tempMaxQueueSizeSetting.get(settings), minSizeKey),
-                Setting.Property.NodeScope);
+            minSizeKey,
+            Integer.toString(minQueueSize),
+            (s) -> Setting.parseInt(s, 0, minSizeKey),
+            new Setting.Validator<Integer>() {
+                @Override
+                public void validate(Integer value, Map<Setting<Integer>, Integer> settings) {
+                    if (value > settings.get(tempMaxQueueSizeSetting)) {
+                        throw new IllegalArgumentException("Failed to parse value [" + value + "] for setting [" + minSizeKey
+                            + "] must be <= " + settings.get(tempMaxQueueSizeSetting));
+                    }
+                }
+
+                @Override
+                public Iterator<Setting<Integer>> settings() {
+                    return Arrays.asList(tempMaxQueueSizeSetting).iterator();
+                }
+            },
+            Setting.Property.NodeScope);
         this.maxQueueSizeSetting = new Setting<>(
                 maxSizeKey,
-                (s) -> Integer.toString(maxQueueSize),
-                (s) -> Setting.parseInt(s, tempMinQueueSizeSetting.get(settings), Integer.MAX_VALUE, maxSizeKey),
+                Integer.toString(maxQueueSize),
+                (s) -> Setting.parseInt(s, 0, maxSizeKey),
+                new Setting.Validator<Integer>() {
+                    @Override
+                    public void validate(Integer value, Map<Setting<Integer>, Integer> settings) {
+                        if (value < settings.get(tempMinQueueSizeSetting)) {
+                            throw new IllegalArgumentException("Failed to parse value [" + value + "] for setting [" + minSizeKey
+                                + "] must be >= " + settings.get(tempMinQueueSizeSetting));
+                        }
+                    }
+
+                    @Override
+                    public Iterator<Setting<Integer>> settings() {
+                        return Arrays.asList(tempMinQueueSizeSetting).iterator();
+                    }
+                },
                 Setting.Property.NodeScope);
         this.frameSizeSetting = Setting.intSetting(frameSizeKey, frameSize, 100, Setting.Property.NodeScope);
     }
@@ -141,12 +171,12 @@ public final class AutoQueueAdjustingExecutorBuilder extends ExecutorBuilder<Aut
 
     static final class AutoExecutorSettings extends ExecutorBuilder.ExecutorSettings {
 
-        private final int size;
-        private final int initialQueueSize;
-        private final int minQueueSize;
-        private final int maxQueueSize;
-        private final int frameSize;
-        private final TimeValue targetedResponseTime;
+        final int size;
+        final int initialQueueSize;
+        final int minQueueSize;
+        final int maxQueueSize;
+        final int frameSize;
+        final TimeValue targetedResponseTime;
 
         AutoExecutorSettings(final String nodeName, final int size, final int initialQueueSize,
                              final int minQueueSize, final int maxQueueSize, final int frameSize,
diff --git a/server/src/test/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilderTests.java b/server/src/test/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilderTests.java
index 836193423f1..ef5f87b940c 100644
--- a/server/src/test/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilderTests.java
+++ b/server/src/test/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilderTests.java
@@ -25,10 +25,10 @@ import static org.hamcrest.CoreMatchers.containsString;
 
 public class AutoQueueAdjustingExecutorBuilderTests extends ESThreadPoolTestCase {
 
-    public void testValidatingMinMaxSettings() throws Exception {
+    public void testValidatingMinMaxSettings() {
         Settings settings = Settings.builder()
-                .put("thread_pool.search.min_queue_size", randomIntBetween(30, 100))
-                .put("thread_pool.search.max_queue_size", randomIntBetween(1,25))
+                .put("thread_pool.test.min_queue_size", randomIntBetween(30, 100))
+                .put("thread_pool.test.max_queue_size", randomIntBetween(1,25))
                 .build();
         try {
             new AutoQueueAdjustingExecutorBuilder(settings, "test", 1, 15, 1, 100, 10);
@@ -36,6 +36,70 @@ public class AutoQueueAdjustingExecutorBuilderTests extends ESThreadPoolTestCase
         } catch (IllegalArgumentException e) {
             assertThat(e.getMessage(), containsString("Failed to parse value"));
         }
+
+        settings = Settings.builder()
+            .put("thread_pool.test.min_queue_size", 10)
+            .put("thread_pool.test.max_queue_size", 9)
+            .build();
+        try {
+            new AutoQueueAdjustingExecutorBuilder(settings, "test", 1, 15, 1, 100, 2000).getSettings(settings);
+            fail("should have thrown an exception");
+        } catch (IllegalArgumentException e) {
+            assertEquals(e.getMessage(), "Failed to parse value [10] for setting [thread_pool.test.min_queue_size] must be <= 9");
+        }
+
+        settings = Settings.builder()
+            .put("thread_pool.test.min_queue_size", 11)
+            .put("thread_pool.test.max_queue_size", 10)
+            .build();
+        try {
+            new AutoQueueAdjustingExecutorBuilder(settings, "test", 1, 15, 1, 100, 2000).getSettings(settings);
+            fail("should have thrown an exception");
+        } catch (IllegalArgumentException e) {
+            assertEquals(e.getMessage(), "Failed to parse value [11] for setting [thread_pool.test.min_queue_size] must be <= 10");
+        }
+
+        settings = Settings.builder()
+            .put("thread_pool.test.min_queue_size", 101)
+            .build();
+        try {
+            new AutoQueueAdjustingExecutorBuilder(settings, "test", 1, 15, 100, 100, 2000).getSettings(settings);
+            fail("should have thrown an exception");
+        } catch (IllegalArgumentException e) {
+            assertEquals(e.getMessage(), "Failed to parse value [101] for setting [thread_pool.test.min_queue_size] must be <= 100");
+        }
+
+        settings = Settings.builder()
+            .put("thread_pool.test.max_queue_size", 99)
+            .build();
+        try {
+            new AutoQueueAdjustingExecutorBuilder(settings, "test", 1, 15, 100, 100, 2000).getSettings(settings);
+            fail("should have thrown an exception");
+        } catch (IllegalArgumentException e) {
+            assertEquals(e.getMessage(), "Failed to parse value [100] for setting [thread_pool.test.min_queue_size] must be <= 99");
+        }
+    }
+
+    public void testSetLowerSettings() {
+        Settings settings = Settings.builder()
+            .put("thread_pool.test.min_queue_size", 10)
+            .put("thread_pool.test.max_queue_size", 10)
+            .build();
+        AutoQueueAdjustingExecutorBuilder test = new AutoQueueAdjustingExecutorBuilder(settings, "test", 1, 1000, 1000, 1000, 2000);
+        AutoQueueAdjustingExecutorBuilder.AutoExecutorSettings s = test.getSettings(settings);
+        assertEquals(10, s.maxQueueSize);
+        assertEquals(10, s.minQueueSize);
+    }
+
+    public void testSetHigherSettings() {
+        Settings settings = Settings.builder()
+            .put("thread_pool.test.min_queue_size", 2000)
+            .put("thread_pool.test.max_queue_size", 3000)
+            .build();
+        AutoQueueAdjustingExecutorBuilder test = new AutoQueueAdjustingExecutorBuilder(settings, "test", 1, 1000, 1000, 1000, 2000);
+        AutoQueueAdjustingExecutorBuilder.AutoExecutorSettings s = test.getSettings(settings);
+        assertEquals(3000, s.maxQueueSize);
+        assertEquals(2000, s.minQueueSize);
     }
 
 }