[GitHub] [accumulo] keith-turner commented on a change in pull request #1950: Clean up CompactionDirectives default impl

[GitBox <gi...@apache.org>]

keith-turner commented on a change in pull request #1950:
URL: https://github.com/apache/accumulo/pull/1950#discussion_r583721202



##########
File path: core/src/main/java/org/apache/accumulo/core/spi/compaction/SimpleCompactionDispatcher.java
##########
@@ -72,19 +72,17 @@
   public void init(InitParameters params) {
     services = new EnumMap<>(CompactionKind.class);
 
-    var defaultService = CompactionDirectives.builder().build();
+    CompactionsDirectiveImpl defaultService = CompactionsDirectiveImpl.DEFAULT;
 
     if (params.getOptions().containsKey("service")) {
-      defaultService =
-          CompactionDirectives.builder().setService(params.getOptions().get("service")).build();
+      defaultService.setService(params.getOptions().get("service"));
     }
 
     for (CompactionKind ctype : CompactionKind.values()) {
       String service = params.getOptions().get("service." + ctype.name().toLowerCase());
-      if (service == null)
-        services.put(ctype, defaultService);
-      else
-        services.put(ctype, CompactionDirectives.builder().setService(service).build());
+      if (service != null)
+        defaultService.setService(service);

Review comment:
       This is changing a static object used by many different dispatcher instance.  This would cause a config change on one table to affect all other tables.  Also the builder used to create immutable objects.

##########
File path: core/src/main/java/org/apache/accumulo/core/spi/compaction/CompactionDirectives.java
##########
@@ -30,19 +30,4 @@
    * @return The service where a compaction should run.
    */
   CompactionServiceId getService();
-
-  /**
-   * @since 2.1.0
-   */
-  public static interface Builder {

Review comment:
       This builder was intended to be used by someone writing a CompactionDispatcher.  If someone were to write a compaction dispatcher now they would need to implement CompactionDirective.

##########
File path: core/src/main/java/org/apache/accumulo/core/spi/compaction/CompactionsDirectiveImpl.java
##########
@@ -18,64 +18,28 @@
  */
 package org.apache.accumulo.core.spi.compaction;
 
-import java.util.Objects;
-
-import org.apache.accumulo.core.spi.compaction.CompactionDirectives.Builder;
-
-import com.google.common.base.Preconditions;
-
 /**
- * This class intentionally package private. This implementation is odd because it supports zero
- * object allocations for {@code CompactionDirectives.builder().build()}.
+ * This class intentionally package private. It supports one object allocation for
+ * {@code CompactionDirectives}.

Review comment:
       ```suggestion
    * This class intentionally package private.
   ```
   
   The old code implemented a copy on write strategy to avoid object allocation when building the default.  If that is to be removed I don't think a comment is needed saying objects are allocated.




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

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