Improve Ample logging for rejected conditional mutations

Author: DomGarguilo

core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java

@@ -24,6 +24,7 @@
 import java.util.Set;
 import java.util.function.Consumer;
 import java.util.function.Predicate;
+import java.util.function.Supplier;
 
 import org.apache.accumulo.core.client.ConditionalWriter;
 import org.apache.accumulo.core.client.admin.TabletAvailability;
@@ -623,6 +624,16 @@ ConditionalTabletMutator requireSame(TabletMetadata tabletMetadata, ColumnType t
      *        let the rejected status carry forward in this case.
      */
     void submit(RejectionHandler rejectionHandler);
+
+    /**
+     * Overloaded version of {@link #submit(RejectionHandler)} that takes a short description of the
+     * operation to assist with debugging.
+     *
+     * @param rejectionHandler The rejection handler
+     * @param description A short description of the operation (e.g., "bulk import", "compaction")
+     */
+    void submit(RejectionHandler rejectionHandler, Supplier<String> description);
+
   }
 
   /**

server/base/src/main/java/org/apache/accumulo/server/metadata/ConditionalTabletMutatorImpl.java

@@ -36,6 +36,7 @@
 import java.util.function.BiConsumer;
 import java.util.function.Consumer;
 import java.util.function.Function;
+import java.util.function.Supplier;
 
 import org.apache.accumulo.core.client.IteratorSetting;
 import org.apache.accumulo.core.client.admin.TabletAvailability;
@@ -86,17 +87,20 @@ public class ConditionalTabletMutatorImpl extends TabletMutatorBase<Ample.Condit
   private final ServerContext context;
   private final ServiceLock lock;
   private final KeyExtent extent;
+  private final BiConsumer<KeyExtent,Supplier<String>> descriptionConsumer;
 
   private boolean sawOperationRequirement = false;
   private boolean checkPrevEndRow = true;
 
   protected ConditionalTabletMutatorImpl(ServerContext context, KeyExtent extent,
       Consumer<ConditionalMutation> mutationConsumer,
-      BiConsumer<KeyExtent,Ample.RejectionHandler> rejectionHandlerConsumer) {
+      BiConsumer<KeyExtent,Ample.RejectionHandler> rejectionHandlerConsumer,
+      BiConsumer<KeyExtent,Supplier<String>> descriptionConsumer) {
     super(new ConditionalMutation(extent.toMetaRow()));
     this.mutation = (ConditionalMutation) super.mutation;
     this.mutationConsumer = mutationConsumer;
     this.rejectionHandlerConsumer = rejectionHandlerConsumer;
+    this.descriptionConsumer = descriptionConsumer;
     this.extent = extent;
     this.context = context;
     this.lock = this.context.getServiceLock();
@@ -390,4 +394,10 @@ public void submit(Ample.RejectionHandler rejectionCheck) {
     mutationConsumer.accept(mutation);
     rejectionHandlerConsumer.accept(extent, rejectionCheck);
   }
+
+  @Override
+  public void submit(Ample.RejectionHandler rejectionHandler, Supplier<String> description) {
+    descriptionConsumer.accept(extent, description);
+    this.submit(rejectionHandler);
+  }
 }

server/base/src/main/java/org/apache/accumulo/server/metadata/ConditionalTabletsMutatorImpl.java

@@ -30,6 +30,7 @@
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.Function;
+import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
 import org.apache.accumulo.core.client.AccumuloException;
@@ -66,6 +67,7 @@ public class ConditionalTabletsMutatorImpl implements Ample.ConditionalTabletsMu
   private boolean active = true;
 
   final Map<KeyExtent,Ample.RejectionHandler> rejectedHandlers = new HashMap<>();
+  private final Map<KeyExtent,Supplier<String>> operationDescriptions = new HashMap<>();
   private final Function<DataLevel,String> tableMapper;
 
   public ConditionalTabletsMutatorImpl(ServerContext context) {
@@ -93,7 +95,8 @@ public Ample.OperationRequirements mutateTablet(KeyExtent extent) {
 
     Preconditions.checkState(extents.putIfAbsent(extent.toMetaRow(), extent) == null,
         "Duplicate extents not handled %s", extent);
-    return new ConditionalTabletMutatorImpl(context, extent, mutations::add, rejectedHandlers::put);
+    return new ConditionalTabletMutatorImpl(context, extent, mutations::add, rejectedHandlers::put,
+        operationDescriptions::put);
   }
 
   protected ConditionalWriter createConditionalWriter(Ample.DataLevel dataLevel)
@@ -262,16 +265,20 @@ public Status getStatus() {
                 status = Status.ACCEPTED;
               }
 
+              Supplier<String> descSupplier = operationDescriptions.get(extent);
+              String desc = (descSupplier == null) ? null : descSupplier.get();
+
               if (log.isTraceEnabled()) {
                 // log detailed info about tablet metadata and mutation
-                log.trace("Mutation was rejected, status:{} {} {}", status, tabletMetadata,
-                    result.getMutation().prettyPrint());
+                log.trace("Mutation was rejected, status:{}. Operation description: {} {} {}",
+                    status, desc, tabletMetadata, result.getMutation().prettyPrint());
               } else if (log.isDebugEnabled()) {
                 // log a single line of info that makes it apparent this happened and gives enough
                 // information to investigate
-                log.debug("Mutation was rejected, status:{} extent:{} row:{}", status,
-                    tabletMetadata == null ? null : tabletMetadata.getExtent(),
-                    new String(result.getMutation().getRow(), UTF_8));
+                log.debug(
+                    "Mutation was rejected, status:{} extent:{} row:{} operation description: {}",
+                    status, tabletMetadata == null ? null : tabletMetadata.getExtent(),
+                    new String(result.getMutation().getRow(), UTF_8), desc);
               }
             }
 

test/src/main/java/org/apache/accumulo/test/functional/AmpleConditionalWriterIT.java

@@ -173,7 +173,8 @@ public void testLocations() throws Exception {
       // test require absent with a future location set
       try (var ctmi = new ConditionalTabletsMutatorImpl(context)) {
         ctmi.mutateTablet(e1).requireAbsentOperation().requireAbsentLocation()
-            .putLocation(Location.future(ts2)).submit(tm -> false);
+            .putLocation(Location.future(ts2)).submit(tm -> false,
+                () -> "Testing that requireAbsentLocation() fails when a future location is set");
         assertEquals(Status.REJECTED, ctmi.process().get(e1).getStatus());
       }
       assertEquals(Location.future(ts1), context.getAmple().readTablet(e1).getLocation());
@@ -196,7 +197,8 @@ public void testLocations() throws Exception {
       try (var ctmi = new ConditionalTabletsMutatorImpl(context)) {
         ctmi.mutateTablet(e1).requireAbsentOperation().requireAbsentLocation()
             .putLocation(Location.future(ts2)).submit(tm -> false);
-        assertEquals(Status.REJECTED, ctmi.process().get(e1).getStatus());
+        assertEquals(Status.REJECTED, ctmi.process().get(e1).getStatus(),
+            () -> "Testing that requireAbsentLocation() fails when a current location is set");
       }
       assertEquals(Location.current(ts1), context.getAmple().readTablet(e1).getLocation());