[GitHub] [accumulo] ctubbsii commented on a change in pull request #2086: Add optional extent to metadata update failure logs

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

ctubbsii commented on a change in pull request #2086:
URL: https://github.com/apache/accumulo/pull/2086#discussion_r628415509



##########
File path: core/src/main/java/org/apache/accumulo/core/data/Mutation.java
##########
@@ -1237,4 +1238,37 @@ protected SERIALIZED_FORMAT getSerializedFormat() {
     return this.useOldDeserialize ? SERIALIZED_FORMAT.VERSION1 : SERIALIZED_FORMAT.VERSION2;
   }
 
+  /**
+   * Creates a multi-lined, human-readable String for this mutation.
+   *
+   * This method creates many intermediate Strings and should not be used for large volumes of
+   * Mutations.
+   *
+   * @return A multi-lined, human-readable String for this mutation.
+   */
+  public static String prettyPrint(Mutation mutation) {

Review comment:
       This is public API, so it would need a `@since` javadoc tag.
   Also, why make this static?
   
   Also, according to semantic versioning, we don't make API additions in bugfix releases, so this should not get added to 1.10, but could be added to 2.1, our next minor release planned.

##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java
##########
@@ -148,22 +148,24 @@ public static void update(ClientContext context, ZooLock zooLock, Mutation m, Ke
   }
 
   public static void update(Writer t, ZooLock zooLock, Mutation m) {
+    update(t, zooLock, m, null);
+  }
+
+  public static void update(Writer t, ZooLock zooLock, Mutation m, KeyExtent extent) {
     if (zooLock != null)
       putLockID(zooLock, m);
     while (true) {
       try {
         t.update(m);
         return;
-      } catch (AccumuloException e) {
-        log.error("{}", e.getMessage(), e);
-      } catch (AccumuloSecurityException e) {
-        log.error("{}", e.getMessage(), e);
+      } catch (AccumuloException | TableNotFoundException | AccumuloSecurityException e) {
+        String extentMsg = extent == null ? "" : " for extent: " + extent;
+        log.error("Failed to write metadata updates {} {}", extentMsg, Mutation.prettyPrint(m), e);
       } catch (ConstraintViolationException e) {
-        log.error("{}", e.getMessage(), e);
+        String extentMsg = extent == null ? "" : " for extent: " + extent;
+        log.error("Failed to write metadata updates {} {}", extentMsg, Mutation.prettyPrint(m), e);

Review comment:
       This code is duplicated. It could be moved to a private static method.

##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java
##########
@@ -34,7 +34,6 @@
 
 import org.apache.accumulo.core.client.admin.TableOperations;
 import org.apache.accumulo.core.conf.AccumuloConfiguration;
-import org.apache.accumulo.core.conf.ConfigurationObserver;

Review comment:
       This seems unrelated. Is this an existing unused import?

##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java
##########
@@ -148,22 +148,24 @@ public static void update(ClientContext context, ZooLock zooLock, Mutation m, Ke
   }
 
   public static void update(Writer t, ZooLock zooLock, Mutation m) {
+    update(t, zooLock, m, null);
+  }
+
+  public static void update(Writer t, ZooLock zooLock, Mutation m, KeyExtent extent) {

Review comment:
       I see the new method here, but I don't see any code that actually uses it, where KeyExtent is non-null.

##########
File path: core/src/main/java/org/apache/accumulo/core/data/Mutation.java
##########
@@ -1237,4 +1238,37 @@ protected SERIALIZED_FORMAT getSerializedFormat() {
     return this.useOldDeserialize ? SERIALIZED_FORMAT.VERSION1 : SERIALIZED_FORMAT.VERSION2;
   }
 
+  /**
+   * Creates a multi-lined, human-readable String for this mutation.
+   *
+   * This method creates many intermediate Strings and should not be used for large volumes of
+   * Mutations.
+   *
+   * @return A multi-lined, human-readable String for this mutation.
+   */
+  public static String prettyPrint(Mutation mutation) {
+    StringBuilder sb = new StringBuilder();
+
+    sb.append("mutation: ").append(utf8String(mutation.row)).append('\n');
+    for (ColumnUpdate update : mutation.getUpdates()) {
+      sb.append(" update: ");
+      sb.append(utf8String(update.getColumnFamily()));
+      sb.append(':');
+      sb.append(utf8String(update.getColumnQualifier()));
+      sb.append(" value ");
+
+      if (update.isDeleted()) {
+        sb.append("[delete]");
+      } else {
+        sb.append(utf8String(update.getValue()));
+      }
+      sb.append('\n');
+    }
+
+    return sb.toString();
+  }
+
+  private static String utf8String(byte[] bytes) {
+    return new String(bytes, StandardCharsets.UTF_8);

Review comment:
       If you just do a static import of `StandardCharsets.UTF_8`, the lines where this is used become short enough that you don't need this separate private helper method.




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