[GitHub] [accumulo] jmark99 commented on a diff in pull request #3199: Added error details for Compaction Configuration failures

["jmark99 (via GitHub)" <gi...@apache.org>]

jmark99 commented on code in PR #3199:
URL: https://github.com/apache/accumulo/pull/3199#discussion_r1114404180


##########
server/base/src/test/java/org/apache/accumulo/server/conf/CheckCompactionConfigTest.java:
##########
@@ -128,6 +128,57 @@ public void testNoPlanner() throws Exception {
     assertTrue(e.getMessage().startsWith(expectedErrorMsg));
   }
 
+  @Test
+  public void testRepeatedCompactionExecutorID() throws Exception {
+    String inputString = ("tserver.compaction.major.service.cs1.planner="
+        + "org.apache.accumulo.core.spi.compaction.DefaultCompactionPlanner \n"
+        + "tserver.compaction.major.service.cs1.planner.opts.executors=\\\n"
+        + "[{'name':'small','type':'internal','maxSize':'16M','numThreads':8},\\\n"
+        + "{'name':'medium','type':'internal','maxSize':'128M','numThreads':4},\\\n"
+        + "{'name':'small','type':'internal','numThreads':2}]").replaceAll("'", "\"");
+    String expectedErrorMsg = "Duplicate Compaction Executor ID found";
+
+    String filePath = writeToFileAndReturnPath(inputString);
+
+    var e = assertThrows(IllegalStateException.class,
+        () -> CheckCompactionConfig.main(new String[] {filePath}));
+    assertTrue(e.getMessage().startsWith(expectedErrorMsg));
+  }
+
+  @Test
+  public void testInvalidTypeValue() throws Exception {
+    String inputString = ("tserver.compaction.major.service.cs1.planner="
+        + "org.apache.accumulo.core.spi.compaction.DefaultCompactionPlanner \n"
+        + "tserver.compaction.major.service.cs1.planner.opts.executors=\\\n"
+        + "[{'name':'small','type':'internal','maxSize':'16M','numThreads':8},\\\n"
+        + "{'name':'medium','type':'internal','maxSize':'128M','numThreads':4},\\\n"
+        + "{'name':'large','type':'internl','numThreads':2}]").replaceAll("'", "\"");
+    String expectedErrorMsg = "type must be 'internal' or 'external'";
+
+    String filePath = writeToFileAndReturnPath(inputString);
+
+    var e = assertThrows(IllegalArgumentException.class,
+        () -> CheckCompactionConfig.main(new String[] {filePath}));
+    assertTrue(e.getMessage().startsWith(expectedErrorMsg));

Review Comment:
   In this particular test class, testing of the message text is used for each of the error checks.The two new tests follow the existing format for the class. I was working with the new compaction service and had an error in my config. I ran it through the check, but the message gave no real information as to what was wrong. It wasn't until I manually found it that I was able to correct it. The additional messaging will hopefully make error discovery easier in the future.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org