Better error when compaction executors are not set (apache#4212)

* Better error when compaction executors are not set

Replaces NPE with proper exception type and message for when the
Default Compaction Planner's "executors" property is set and empty 
or not set at all.

Author: ddanielr

core/src/main/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlanner.java

@@ -167,62 +167,68 @@ public String toString() {
       justification = "Field is written by Gson")
   @Override
   public void init(InitParameters params) {
-    ExecutorConfig[] execConfigs =
-        new Gson().fromJson(params.getOptions().get("executors"), ExecutorConfig[].class);
 
-    List<Executor> tmpExec = new ArrayList<>();
+    if (params.getOptions().containsKey("executors")
+        && !params.getOptions().get("executors").isBlank()) {
 
-    for (ExecutorConfig executorConfig : execConfigs) {
-      Long maxSize = executorConfig.maxSize == null ? null
-          : ConfigurationTypeHelper.getFixedMemoryAsBytes(executorConfig.maxSize);
+      ExecutorConfig[] execConfigs =
+          new Gson().fromJson(params.getOptions().get("executors"), ExecutorConfig[].class);
 
-      CompactionExecutorId ceid;
+      List<Executor> tmpExec = new ArrayList<>();
 
-      // If not supplied, GSON will leave type null. Default to internal
-      if (executorConfig.type == null) {
-        executorConfig.type = "internal";
-      }
+      for (ExecutorConfig executorConfig : execConfigs) {
+        Long maxSize = executorConfig.maxSize == null ? null
+            : ConfigurationTypeHelper.getFixedMemoryAsBytes(executorConfig.maxSize);
 
-      switch (executorConfig.type) {
-        case "internal":
-          Preconditions.checkArgument(null == executorConfig.queue,
-              "'queue' should not be specified for internal compactions");
-          int numThreads = Objects.requireNonNull(executorConfig.numThreads,
-              "'numThreads' must be specified for internal type");
-          ceid = params.getExecutorManager().createExecutor(executorConfig.name, numThreads);
-          break;
-        case "external":
-          Preconditions.checkArgument(null == executorConfig.numThreads,
-              "'numThreads' should not be specified for external compactions");
-          String queue = Objects.requireNonNull(executorConfig.queue,
-              "'queue' must be specified for external type");
-          ceid = params.getExecutorManager().getExternalExecutor(queue);
-          break;
-        default:
-          throw new IllegalArgumentException("type must be 'internal' or 'external'");
-      }
-      tmpExec.add(new Executor(ceid, maxSize));
-    }
+        CompactionExecutorId ceid;
 
-    Collections.sort(tmpExec, Comparator.comparing(Executor::getMaxSize,
-        Comparator.nullsLast(Comparator.naturalOrder())));
+        // If not supplied, GSON will leave type null. Default to internal
+        if (executorConfig.type == null) {
+          executorConfig.type = "internal";
+        }
 
-    executors = List.copyOf(tmpExec);
+        switch (executorConfig.type) {
+          case "internal":
+            Preconditions.checkArgument(null == executorConfig.queue,
+                "'queue' should not be specified for internal compactions");
+            int numThreads = Objects.requireNonNull(executorConfig.numThreads,
+                "'numThreads' must be specified for internal type");
+            ceid = params.getExecutorManager().createExecutor(executorConfig.name, numThreads);
+            break;
+          case "external":
+            Preconditions.checkArgument(null == executorConfig.numThreads,
+                "'numThreads' should not be specified for external compactions");
+            String queue = Objects.requireNonNull(executorConfig.queue,
+                "'queue' must be specified for external type");
+            ceid = params.getExecutorManager().getExternalExecutor(queue);
+            break;
+          default:
+            throw new IllegalArgumentException("type must be 'internal' or 'external'");
+        }
+        tmpExec.add(new Executor(ceid, maxSize));
+      }
 
-    if (executors.stream().filter(e -> e.getMaxSize() == null).count() > 1) {
-      throw new IllegalArgumentException(
-          "Can only have one executor w/o a maxSize. " + params.getOptions().get("executors"));
-    }
+      Collections.sort(tmpExec, Comparator.comparing(Executor::getMaxSize,
+          Comparator.nullsLast(Comparator.naturalOrder())));
+
+      executors = List.copyOf(tmpExec);
 
-    // use the add method on the Set interface to check for duplicate maxSizes
-    Set<Long> maxSizes = new HashSet<>();
-    executors.forEach(e -> {
-      if (!maxSizes.add(e.getMaxSize())) {
+      if (executors.stream().filter(e -> e.getMaxSize() == null).count() > 1) {
         throw new IllegalArgumentException(
-            "Duplicate maxSize set in executors. " + params.getOptions().get("executors"));
+            "Can only have one executor w/o a maxSize. " + params.getOptions().get("executors"));
       }
-    });
 
+      // use the add method on the Set interface to check for duplicate maxSizes
+      Set<Long> maxSizes = new HashSet<>();
+      executors.forEach(e -> {
+        if (!maxSizes.add(e.getMaxSize())) {
+          throw new IllegalArgumentException(
+              "Duplicate maxSize set in executors. " + params.getOptions().get("executors"));
+        }
+      });
+    } else {
+      throw new IllegalStateException("No defined executors for this planner");
+    }
     determineMaxFilesToCompact(params);
   }
 

core/src/test/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlannerTest.java

@@ -470,6 +470,40 @@ public void testErrorDuplicateMaxSize() {
     assertTrue(e.getMessage().contains("maxSize"), "Error message didn't contain maxSize");
   }
 
+  /**
+   * Tests when "executors" is defined but empty.
+   */
+  @Test
+  public void testErrorEmptyExecutors() {
+    DefaultCompactionPlanner planner = new DefaultCompactionPlanner();
+    String executors = "";
+
+    var e = assertThrows(IllegalStateException.class,
+        () -> planner.init(getInitParams(defaultConf, executors)), "Failed to throw error");
+    assertTrue(e.getMessage().contains("No defined executors"),
+        "Error message didn't contain 'No defined executors'");
+  }
+
+  /**
+   * Tests when "executors" doesn't exist
+   */
+  @Test
+  public void testErrorNoExecutors() {
+
+    ServiceEnvironment senv = EasyMock.createMock(ServiceEnvironment.class);
+    EasyMock.expect(senv.getConfiguration()).andReturn(defaultConf).anyTimes();
+    EasyMock.replay(senv);
+
+    var initParams = new CompactionPlannerInitParams(csid, new HashMap<>(), senv);
+
+    DefaultCompactionPlanner dcPlanner = new DefaultCompactionPlanner();
+
+    var e = assertThrows(IllegalStateException.class, () -> dcPlanner.init(initParams),
+        "Failed to throw error");
+    assertTrue(e.getMessage().contains("No defined executors"),
+        "Error message didn't contain 'No defined executors'");
+  }
+
   // Test cases where a tablet has more than table.file.max files, but no files were found using the
   // compaction ratio. The planner should try to find the highest ratio that will result in a
   // compaction.