[GitHub] [accumulo] keith-turner commented on a diff in pull request #3773: Remove unneeded chop compaction and split code

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

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


##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/UpgraderConstants.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.manager.upgrade;
+
+import org.apache.accumulo.core.util.ColumnFQ;
+import org.apache.hadoop.io.Text;
+
+public class UpgraderConstants {
+
+  /**
+   * ChoppedColumnFamily kept around for cleaning up old entries on upgrade

Review Comment:
   ```suggestion
      * ChoppedColumnFamily kept around for cleaning up old entries on upgrade.  Currently not used, will be used by #3768
   ```



##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/MajCEnv.java:
##########
@@ -85,8 +85,6 @@ public TCompactionReason getReason() {
     switch (kind) {
       case USER:
         return TCompactionReason.USER;
-      case CHOP:
-        return TCompactionReason.CHOP;
       case SELECTOR:
       case SYSTEM:
       default:

Review Comment:
   This was preexisting code, but the way the default case in the code works seems kinda suspect.  I will open an issue.



##########
core/src/main/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlanner.java:
##########
@@ -272,8 +272,7 @@ public CompactionPlan makePlan(PlanningParameters params) {
     }
 
     if (group.isEmpty()
-        && (params.getKind() == CompactionKind.USER || params.getKind() == CompactionKind.SELECTOR
-            || params.getKind() == CompactionKind.CHOP)

Review Comment:
   For completeness, should still handle this case in some way.   Experimented locally with the following changes to not use chop in the code.  Also made this default planner throw an exception if it sees chopped.  
   
   ```diff
   diff --git a/core/src/main/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlanner.java b/core/src/main/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlanner.java
   index ad512da818..8fffeea6bf 100644
   --- a/core/src/main/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlanner.java
   +++ b/core/src/main/java/org/apache/accumulo/core/spi/compaction/DefaultCompactionPlanner.java
   @@ -215,6 +215,8 @@ public class DefaultCompactionPlanner implements CompactionPlanner {
          return params.createPlanBuilder().build();
        }
    
   +    Preconditions.checkArgument(params.getKind() != CompactionKind.CHOP);
   +
        Set<CompactableFile> filesCopy = new HashSet<>(params.getCandidates());
    
        long maxSizeToCompact = getMaxSizeToCompact(params.getKind());
   diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionManager.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionManager.java
   index 9957eb1dee..87ace5ff4c 100644
   --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionManager.java
   +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionManager.java
   @@ -150,6 +150,10 @@ public class CompactionManager {
       */
      private void submitCompaction(Compactable compactable) {
        for (CompactionKind ctype : CompactionKind.values()) {
   +      if(ctype == CompactionKind.CHOP) {
   +        continue;
   +      }
   +
          var csid = compactable.getConfiguredService(ctype);
          var service = services.get(csid);
          if (service == null) {
   diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionService.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionService.java
   index 1d1b0f0e8f..14c8bf7543 100644
   --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionService.java
   +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionService.java
   @@ -268,6 +268,8 @@ public class CompactionService {
    
      private CompactionPlan getCompactionPlan(CompactionKind kind, Compactable.Files files,
          Compactable compactable) {
   +    Preconditions.checkArgument(kind != CompactionKind.CHOP, "Chop compactions are no longer expected");
   +
        PlanningParameters params = new CpPlanParams(kind, compactable, files);
    
        log.trace("Planning compactions {} {} {} {}", planner.getClass().getName(),
   ```
   
   Wondering if it would be better to remove the CHOP enum.  We could make this change to the SPI.



##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -397,34 +309,8 @@ Set<StoredTabletFile> getCandidates(Set<StoredTabletFile> currFiles, CompactionK
           // intentional fall through
         case USER:
           return handleUserSelectorCompaction(currFiles, kind);
-        case CHOP: {
-          return handleChopCompaction(currFiles);
-        }
-        default:
-          throw new AssertionError();
-      }
-    }
-
-    private Set<StoredTabletFile> handleChopCompaction(Set<StoredTabletFile> currFiles) {
-      switch (chopStatus) {
-        case NOT_ACTIVE:
-        case SELECTING:
-        case MARKING:
+        case CHOP:
           return Set.of();

Review Comment:
   ```suggestion
             throw new IllegalArgumentException("Chop compactions are no longer expected");
   ```



##########
server/compactor/src/main/java/org/apache/accumulo/compactor/ExtCEnv.java:
##########
@@ -102,8 +102,6 @@ public TCompactionReason getReason() {
     switch (job.getKind()) {
       case USER:
         return TCompactionReason.USER;
-      case CHOP:
-        return TCompactionReason.CHOP;
       case SELECTOR:
       case SYSTEM:
       default:

Review Comment:
   This default case seems suspicious, will open an issue about it.



##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -647,7 +647,6 @@ TabletGoalState getGoalState(TabletLocationState tls, MergeInfo mergeInfo) {
             case COMPLETE:
               break;
             case STARTED:
-            case SPLITTING:

Review Comment:
   When splitting used to be done we needed to host tablets in order to split them.   Now that splitting is no longer needed tablets need to be unhosted.  Wondering if the merge code will host a tablet and then unhost here.  Not sure how the larger code around these states works.



##########
server/manager/src/main/java/org/apache/accumulo/manager/state/MergeStats.java:
##########
@@ -133,34 +109,16 @@ public MergeState nextMergeState(AccumuloClient accumuloClient, CurrentState man
         log.info("Waiting for {} hosted tablets to be {} {}", hosted, total, info.getExtent());

Review Comment:
   Wonder if this code is needlessly waiting for tablet to be hosted.



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