[accumulo] branch main updated: Create Ample interface method for createDeleteMutation (#1757)

[mm...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

mmiller pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/accumulo.git


The following commit(s) were added to refs/heads/main by this push:
     new d820360  Create Ample interface method for createDeleteMutation (#1757)
d820360 is described below

commit d820360b975de759b1575d68456e69c828225a85
Author: Mike Miller <mm...@apache.org>
AuthorDate: Fri Oct 30 12:26:48 2020 -0400

    Create Ample interface method for createDeleteMutation (#1757)
    
    * Drop static method in favor of method on the Ample interface for
    creating delete mutations
    * Change GarbageCollectorIT to use Ample for one of the tests
    * Reuse Ample object in loops to prevent extra object creation
    * Further improvements could be made to not have the Mutation type in Ample
---
 .../org/apache/accumulo/core/metadata/TabletFileUtil.java |  3 ++-
 .../org/apache/accumulo/core/metadata/schema/Ample.java   | 15 +++++++++++++++
 .../apache/accumulo/server/metadata/ServerAmpleImpl.java  | 11 ++++++-----
 .../apache/accumulo/server/util/MetadataTableUtil.java    | 14 ++++++++------
 .../org/apache/accumulo/master/TabletGroupWatcher.java    | 13 ++++++++-----
 .../org/apache/accumulo/master/upgrade/Upgrader9to10.java |  4 ++--
 .../accumulo/test/functional/GarbageCollectorIT.java      |  6 +++---
 7 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/core/src/main/java/org/apache/accumulo/core/metadata/TabletFileUtil.java b/core/src/main/java/org/apache/accumulo/core/metadata/TabletFileUtil.java
index 372c017..ff92ae3 100644
--- a/core/src/main/java/org/apache/accumulo/core/metadata/TabletFileUtil.java
+++ b/core/src/main/java/org/apache/accumulo/core/metadata/TabletFileUtil.java
@@ -28,7 +28,8 @@ public class TabletFileUtil {
   /**
    * Validate if string is a valid path. Return normalized string or throw exception if not valid.
    * This was added to facilitate more use of TabletFile over String but this puts the validation in
-   * one location in the case where TabletFile can't be used.
+   * one location in the case where TabletFile can't be used. The Garbage Collector is optimized to
+   * store a directory for Tablet File so a String is used.
    */
   public static String validate(String path) {
     Path p = new Path(path);
diff --git a/core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java b/core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java
index 2a33e17..4703b32 100644
--- a/core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java
+++ b/core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java
@@ -21,6 +21,7 @@ package org.apache.accumulo.core.metadata.schema;
 import java.util.Collection;
 import java.util.Iterator;
 
+import org.apache.accumulo.core.data.Mutation;
 import org.apache.accumulo.core.data.TableId;
 import org.apache.accumulo.core.dataImpl.KeyExtent;
 import org.apache.accumulo.core.metadata.MetadataTable;
@@ -152,6 +153,20 @@ public interface Ample {
   }
 
   /**
+   * Return an encoded delete marker Mutation to delete the specified TabletFile path. A String is
+   * used for the parameter because the Garbage Collector is optimized to store a directory for
+   * Tablet File. Otherwise a {@link TabletFile} object could be used. The tabletFilePathToRemove is
+   * validated and normalized before creating the mutation.
+   *
+   * @param tabletFilePathToRemove
+   *          String full path of the TabletFile
+   * @return Mutation with encoded delete marker
+   */
+  default Mutation createDeleteMutation(String tabletFilePathToRemove) {
+    throw new UnsupportedOperationException();
+  }
+
+  /**
    * This interface allows efficiently updating multiple tablets. Unless close is called, changes
    * may not be persisted.
    */
diff --git a/server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java b/server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java
index c42715e..b1d89dd 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/metadata/ServerAmpleImpl.java
@@ -195,18 +195,19 @@ public class ServerAmpleImpl extends AmpleImpl implements Ample {
     }
   }
 
-  public static Mutation createDeleteMutation(String pathToRemove) {
-    String path = TabletFileUtil.validate(pathToRemove);
+  @Override
+  public Mutation createDeleteMutation(String tabletFilePathToRemove) {
+    String path = TabletFileUtil.validate(tabletFilePathToRemove);
     return createDelMutation(path);
   }
 
-  public static Mutation createDeleteMutation(StoredTabletFile pathToRemove) {
+  public Mutation createDeleteMutation(StoredTabletFile pathToRemove) {
     return createDelMutation(pathToRemove.getMetaUpdateDelete());
   }
 
-  private static Mutation createDelMutation(String path) {
+  private Mutation createDelMutation(String path) {
     Mutation delFlag = new Mutation(new Text(DeletesSection.encodeRow(path)));
-    delFlag.put(EMPTY_TEXT, EMPTY_TEXT, SkewedKeyValue.NAME);
+    delFlag.put(EMPTY_TEXT, EMPTY_TEXT, DeletesSection.SkewedKeyValue.NAME);
     return delFlag;
   }
 }
diff --git a/server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java b/server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java
index e01323c..a8c6011 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java
@@ -67,6 +67,7 @@ import org.apache.accumulo.core.metadata.RootTable;
 import org.apache.accumulo.core.metadata.StoredTabletFile;
 import org.apache.accumulo.core.metadata.TabletFile;
 import org.apache.accumulo.core.metadata.TabletFileUtil;
+import org.apache.accumulo.core.metadata.schema.Ample;
 import org.apache.accumulo.core.metadata.schema.Ample.TabletMutator;
 import org.apache.accumulo.core.metadata.schema.DataFileValue;
 import org.apache.accumulo.core.metadata.schema.MetadataSchema.BlipSection;
@@ -93,7 +94,6 @@ import org.apache.accumulo.fate.FateTxId;
 import org.apache.accumulo.fate.zookeeper.ZooLock;
 import org.apache.accumulo.server.ServerContext;
 import org.apache.accumulo.server.gc.GcVolumeUtil;
-import org.apache.accumulo.server.metadata.ServerAmpleImpl;
 import org.apache.hadoop.io.Text;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -283,17 +283,18 @@ public class MetadataTableUtil {
    * datafilesToDelete are strings because they can be a TabletFile or directory
    */
   public static void addDeleteEntries(KeyExtent extent, Set<String> datafilesToDelete,
-      ServerContext context) {
+      ServerContext context, Ample ample) {
 
     // TODO could use batch writer,would need to handle failure and retry like update does -
     // ACCUMULO-1294
     for (String pathToRemove : datafilesToDelete) {
-      update(context, ServerAmpleImpl.createDeleteMutation(pathToRemove), extent);
+      update(context, ample.createDeleteMutation(pathToRemove), extent);
     }
   }
 
   public static void addDeleteEntry(ServerContext context, TableId tableId, String path) {
-    update(context, ServerAmpleImpl.createDeleteMutation(path), new KeyExtent(tableId, null, null));
+    update(context, context.getAmple().createDeleteMutation(path),
+        new KeyExtent(tableId, null, null));
   }
 
   public static void removeScanFiles(KeyExtent extent, Set<StoredTabletFile> scanFiles,
@@ -364,6 +365,7 @@ public class MetadataTableUtil {
 
       // scan metadata for our table and delete everything we find
       Mutation m = null;
+      Ample ample = context.getAmple();
       ms.setRange(new KeyExtent(tableId, null, null).toMetaRange());
 
       // insert deletes before deleting data from metadata... this makes the code fault tolerant
@@ -377,13 +379,13 @@ public class MetadataTableUtil {
 
           if (key.getColumnFamily().equals(DataFileColumnFamily.NAME)) {
             String ref = TabletFileUtil.validate(key.getColumnQualifierData().toString());
-            bw.addMutation(ServerAmpleImpl.createDeleteMutation(ref));
+            bw.addMutation(ample.createDeleteMutation(ref));
           }
 
           if (ServerColumnFamily.DIRECTORY_COLUMN.hasColumns(key)) {
             String uri =
                 GcVolumeUtil.getDeleteTabletOnAllVolumesUri(tableId, cell.getValue().toString());
-            bw.addMutation(ServerAmpleImpl.createDeleteMutation(uri));
+            bw.addMutation(ample.createDeleteMutation(uri));
           }
         }
 
diff --git a/server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java b/server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
index c661f85..5647554 100644
--- a/server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
+++ b/server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
@@ -59,6 +59,7 @@ import org.apache.accumulo.core.master.thrift.TabletServerStatus;
 import org.apache.accumulo.core.metadata.MetadataTable;
 import org.apache.accumulo.core.metadata.RootTable;
 import org.apache.accumulo.core.metadata.TabletFileUtil;
+import org.apache.accumulo.core.metadata.schema.Ample;
 import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection;
 import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.ChoppedColumnFamily;
 import org.apache.accumulo.core.metadata.schema.MetadataSchema.TabletsSection.CurrentLocationColumnFamily;
@@ -74,6 +75,7 @@ import org.apache.accumulo.master.Master.TabletGoalState;
 import org.apache.accumulo.master.state.MergeStats;
 import org.apache.accumulo.master.state.TableCounts;
 import org.apache.accumulo.master.state.TableStats;
+import org.apache.accumulo.server.ServerContext;
 import org.apache.accumulo.server.conf.TableConfiguration;
 import org.apache.accumulo.server.gc.GcVolumeUtil;
 import org.apache.accumulo.server.log.WalStateManager;
@@ -89,7 +91,6 @@ import org.apache.accumulo.server.master.state.TabletLocationState;
 import org.apache.accumulo.server.master.state.TabletLocationState.BadLocationStateException;
 import org.apache.accumulo.server.master.state.TabletState;
 import org.apache.accumulo.server.master.state.TabletStateStore;
-import org.apache.accumulo.server.metadata.ServerAmpleImpl;
 import org.apache.accumulo.server.tablets.TabletTime;
 import org.apache.accumulo.server.util.MetadataTableUtil;
 import org.apache.hadoop.fs.Path;
@@ -584,6 +585,8 @@ abstract class TabletGroupWatcher extends Daemon {
     }
     try {
       AccumuloClient client = master.getContext();
+      ServerContext context = master.getContext();
+      Ample ample = context.getAmple();
       Text start = extent.prevEndRow();
       if (start == null) {
         start = new Text();
@@ -603,7 +606,7 @@ abstract class TabletGroupWatcher extends Daemon {
         if (key.compareColumnFamily(DataFileColumnFamily.NAME) == 0) {
           datafiles.add(TabletFileUtil.validate(key.getColumnQualifierData().toString()));
           if (datafiles.size() > 1000) {
-            MetadataTableUtil.addDeleteEntries(extent, datafiles, master.getContext());
+            MetadataTableUtil.addDeleteEntries(extent, datafiles, context, ample);
             datafiles.clear();
           }
         } else if (ServerColumnFamily.TIME_COLUMN.hasColumns(key)) {
@@ -616,12 +619,12 @@ abstract class TabletGroupWatcher extends Daemon {
               entry.getValue().toString());
           datafiles.add(path);
           if (datafiles.size() > 1000) {
-            MetadataTableUtil.addDeleteEntries(extent, datafiles, master.getContext());
+            MetadataTableUtil.addDeleteEntries(extent, datafiles, context, ample);
             datafiles.clear();
           }
         }
       }
-      MetadataTableUtil.addDeleteEntries(extent, datafiles, master.getContext());
+      MetadataTableUtil.addDeleteEntries(extent, datafiles, context, ample);
       BatchWriter bw = client.createBatchWriter(targetSystemTable, new BatchWriterConfig());
       try {
         deleteTablets(info, deleteRange, bw, client);
@@ -700,7 +703,7 @@ abstract class TabletGroupWatcher extends Daemon {
         } else if (ServerColumnFamily.DIRECTORY_COLUMN.hasColumns(key)) {
           String uri =
               GcVolumeUtil.getDeleteTabletOnAllVolumesUri(range.tableId(), value.toString());
-          bw.addMutation(ServerAmpleImpl.createDeleteMutation(uri));
+          bw.addMutation(master.getContext().getAmple().createDeleteMutation(uri));
         }
       }
 
diff --git a/server/manager/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java b/server/manager/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java
index eeae1c6..f7c6d1a 100644
--- a/server/manager/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java
+++ b/server/manager/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java
@@ -78,7 +78,6 @@ import org.apache.accumulo.server.fs.VolumeManager;
 import org.apache.accumulo.server.gc.GcVolumeUtil;
 import org.apache.accumulo.server.master.state.TServerInstance;
 import org.apache.accumulo.server.metadata.RootGcCandidates;
-import org.apache.accumulo.server.metadata.ServerAmpleImpl;
 import org.apache.accumulo.server.metadata.TabletMutatorBase;
 import org.apache.accumulo.server.util.TablePropUtil;
 import org.apache.hadoop.fs.FileStatus;
@@ -424,6 +423,7 @@ public class Upgrader9to10 implements Upgrader {
 
     String tableName = level.metaTable();
     AccumuloClient c = ctx;
+    Ample ample = ctx.getAmple();
 
     // find all deletes
     try (BatchWriter writer = c.createBatchWriter(tableName, new BatchWriterConfig())) {
@@ -441,7 +441,7 @@ public class Upgrader9to10 implements Upgrader {
           Path absolutePath = resolveRelativeDelete(olddelete, upgradeProp);
           String updatedDel = switchToAllVolumes(absolutePath);
 
-          writer.addMutation(ServerAmpleImpl.createDeleteMutation(updatedDel));
+          writer.addMutation(ample.createDeleteMutation(updatedDel));
         }
         writer.flush();
         // if nothing thrown then we're good so mark all deleted
diff --git a/test/src/main/java/org/apache/accumulo/test/functional/GarbageCollectorIT.java b/test/src/main/java/org/apache/accumulo/test/functional/GarbageCollectorIT.java
index d78465f..588ab68 100644
--- a/test/src/main/java/org/apache/accumulo/test/functional/GarbageCollectorIT.java
+++ b/test/src/main/java/org/apache/accumulo/test/functional/GarbageCollectorIT.java
@@ -44,6 +44,7 @@ import org.apache.accumulo.core.data.Key;
 import org.apache.accumulo.core.data.Mutation;
 import org.apache.accumulo.core.data.Value;
 import org.apache.accumulo.core.metadata.MetadataTable;
+import org.apache.accumulo.core.metadata.schema.Ample;
 import org.apache.accumulo.core.metadata.schema.MetadataSchema.DeletesSection;
 import org.apache.accumulo.core.metadata.schema.MetadataSchema.DeletesSection.SkewedKeyValue;
 import org.apache.accumulo.core.security.Authorizations;
@@ -60,7 +61,6 @@ import org.apache.accumulo.miniclusterImpl.MiniAccumuloClusterImpl.ProcessInfo;
 import org.apache.accumulo.miniclusterImpl.MiniAccumuloConfigImpl;
 import org.apache.accumulo.miniclusterImpl.ProcessNotFoundException;
 import org.apache.accumulo.miniclusterImpl.ProcessReference;
-import org.apache.accumulo.server.metadata.ServerAmpleImpl;
 import org.apache.accumulo.test.TestIngest;
 import org.apache.accumulo.test.VerifyIngest;
 import org.apache.accumulo.test.VerifyIngest.VerifyParams;
@@ -305,14 +305,14 @@ public class GarbageCollectorIT extends ConfigurableMacBase {
   }
 
   private void addEntries(AccumuloClient client) throws Exception {
+    Ample ample = getServerContext().getAmple();
     client.securityOperations().grantTablePermission(client.whoami(), MetadataTable.NAME,
         TablePermission.WRITE);
     try (BatchWriter bw = client.createBatchWriter(MetadataTable.NAME)) {
       for (int i = 0; i < 100000; ++i) {
         String longpath = "aaaaaaaaaabbbbbbbbbbccccccccccddddddddddeeeeeeeeee"
             + "ffffffffffgggggggggghhhhhhhhhhiiiiiiiiiijjjjjjjjjj";
-        Mutation delFlag =
-            ServerAmpleImpl.createDeleteMutation(String.format("file:/%020d/%s", i, longpath));
+        Mutation delFlag = ample.createDeleteMutation(String.format("file:/%020d/%s", i, longpath));
         bw.addMutation(delFlag);
       }
     }