Re: [PR] Remove old compaction code [accumulo]

["keith-turner (via GitHub)" <gi...@apache.org>]

keith-turner commented on code in PR #4083:
URL: https://github.com/apache/accumulo/pull/4083#discussion_r1441163615


##########
core/src/test/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlannerTest.java:
##########
@@ -433,114 +424,57 @@ public void testUserCompactionDoesNotWaitOnSystemCompaction() {
   }
 
   @Test
-  public void testQueueCreation() throws Exception {
+  public void testQueueCreation() {
     DefaultCompactionPlanner planner = new DefaultCompactionPlanner();
 
-    String queues = "[{\"name\": \"small\", \"maxSize\":\"32M\"},{\"name\":\"midsize\"}]";
-    planner.init(getInitParamQueues(defaultConf, queues));
+    String groups = "[{\"name\": \"small\", \"maxSize\":\"32M\"},{\"name\":\"midsize\"}]";
+    planner.init(getInitParams(defaultConf, groups));
 
     var all = createCFs("F1", "1M", "F2", "1M", "F3", "1M", "F4", "1M");
     var params = createPlanningParams(all, all, Set.of(), 2, CompactionKind.SYSTEM);
     var plan = planner.makePlan(params);
 
     var job = getOnlyElement(plan.getJobs());
     assertEquals(all, job.getFiles());
-    assertEquals(CompactionExecutorIdImpl.externalId("small"), job.getExecutor());
+    assertEquals(CompactionGroupIdImpl.groupId("small"), job.getGroup());
 
     all = createCFs("F1", "100M", "F2", "100M", "F3", "100M", "F4", "100M");
     params = createPlanningParams(all, all, Set.of(), 2, CompactionKind.SYSTEM);
     plan = planner.makePlan(params);
 
     job = getOnlyElement(plan.getJobs());
     assertEquals(all, job.getFiles());
-    assertEquals(CompactionExecutorIdImpl.externalId("midsize"), job.getExecutor());
+    assertEquals(CompactionGroupIdImpl.groupId("midsize"), job.getGroup());
   }
 
   /**
    * Tests that additional fields in the JSON objects cause errors to be thrown.
    */
   @Test
   public void testErrorAdditionalConfigFields() {
-    DefaultCompactionPlanner QueuePlanner = new DefaultCompactionPlanner();
+    DefaultCompactionPlanner planner = new DefaultCompactionPlanner();
 
-    String queues =
+    String groups =
         "[{\"name\":\"smallQueue\", \"maxSize\":\"32M\"}, {\"name\":\"largeQueue\", \"type\":\"internal\", \"foo\":\"bar\", \"queue\":\"broken\"}]";
 
-    final InitParameters queueParams = getInitParamQueues(defaultConf, queues);
-    assertNotNull(queueParams);
-    var e = assertThrows(JsonParseException.class, () -> QueuePlanner.init(queueParams),
-        "Failed to throw error");
+    final InitParameters params = getInitParams(defaultConf, groups);

Review Comment:
   This test is much simpler now.



##########
core/src/main/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlanner.java:
##########
@@ -58,85 +58,48 @@
  * compaction service you are configuring.
  *
  * <ul>
- * <li>{@code compaction.service.<service>.opts.executors} This is a json array of objects where
- * each object has the fields:
+ * <li>Note that the CompactionCoordinator and at least one Compactor for the "large" compaction

Review Comment:
   Thinking we may want to use the terminology "compactor group" when expanding "group".  If that sounds good there are a few other places that could change.
   
   ```suggestion
    * <li>Note that the CompactionCoordinator and at least one Compactor for the "large" compactor
   ```



##########
core/src/main/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlanner.java:
##########
@@ -58,85 +58,48 @@
  * compaction service you are configuring.
  *
  * <ul>
- * <li>{@code compaction.service.<service>.opts.executors} This is a json array of objects where
- * each object has the fields:
+ * <li>Note that the CompactionCoordinator and at least one Compactor for the "large" compaction
+ * group must be running.
+ * <li>{@code compaction.service.<service>.opts.maxOpen} This determines the maximum number of files
+ * that will be included in a single compaction.
+ * <li>{@code compaction.service.<service>.opts.groups} This is a json array of group objects which
+ * have the following fields:
  * <table>
- * <caption>Default Compaction Planner Executor options</caption>
+ * <caption>Default Compaction Planner Group options</caption>
  * <tr>
  * <th>Field Name</th>
  * <th>Description</th>
  * </tr>
  * <tr>
  * <td>name</td>
- * <td>name or alias of the executor (required)</td>
- * </tr>
- * <tr>
- * <td>type</td>
- * <td>valid values 'internal' or 'external' (required)</td>
+ * <td>name of the group (required)</td>
  * </tr>
  * <tr>
  * <td>maxSize</td>
  * <td>threshold sum of the input files (required for all but one of the configs)</td>
  * </tr>
- * <tr>
- * <td>numThreads</td>
- * <td>number of threads for this executor configuration (required for 'internal', cannot be
- * specified for 'external')</td>
- * </tr>
- * <tr>
- * <td>group</td>
- * <td>name of the external compaction group (required for 'external', cannot be specified for
- * 'internal')</td>
- * </tr>
  * </table>
  * <br>
- * Note: The "executors" option has been deprecated in 3.1 and will be removed in a future release.
- * This example uses the new `compaction.service` prefix. The property prefix
- * "tserver.compaction.major.service" has also been deprecated in 3.1 and will be removed in a
- * future release. The maxSize field determines the maximum size of compaction that will run on an
- * executor. The maxSize field can have a suffix of K,M,G for kilobytes, megabytes, or gigabytes and
- * represents the sum of the input files for a given compaction. One executor can have no max size
- * and it will run everything that is too large for the other executors. If all executors have a max
- * size, then system compactions will only run for compactions smaller than the largest max size.
- * User, chop, and selector compactions will always run, even if there is no executor for their
- * size. These compactions will run on the executor with the largest max size. The following example
- * value for this property will create 3 threads to run compactions of files whose file size sum is
- * less than 100M, 3 threads to run compactions of files whose file size sum is less than 500M, and
- * run all other compactions on Compactors configured to run compactions for Queue1:
+ * This 'groups' object is used for defining compaction groups. The maxSize field determines the

Review Comment:
   ```suggestion
    * This 'groups' object provides information that is used for mapping a compaction job to a compactor group. The maxSize field determines the
   ```



-- 
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