You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/05/07 12:53:51 UTC
[GitHub] [accumulo] jschmidt10 opened a new pull request #2086: Add optional extent to metadata update failure logs
jschmidt10 opened a new pull request #2086:
URL: https://github.com/apache/accumulo/pull/2086
This week we saw many "Failed to do close consistency check" errors. The log showed that the accumulo.metadata table had additional RFiles that were not present in the tserver's DatafileManager.datafileSizes map. We believe that this may have been due to a temporary network drop where the importing tserver sent the accumulo.metadata updates - the metadata updates were persisted but the importing tserver's writer client did not receive a successful response.
We'd like to add some additional logging to MetadataTableUtil so that we can tie the failures back to particular tablets to see if these correlate.
--
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
[GitHub] [accumulo] jschmidt10 commented on a change in pull request #2086: Add optional extent to metadata update failure logs
Posted by GitBox <gi...@apache.org>.
jschmidt10 commented on a change in pull request #2086:
URL: https://github.com/apache/accumulo/pull/2086#discussion_r629379374
##########
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:
Good catch, I missed an update.
--
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
[GitHub] [accumulo] jschmidt10 commented on a change in pull request #2086: Add optional extent to metadata update failure logs
Posted by GitBox <gi...@apache.org>.
jschmidt10 commented on a change in pull request #2086:
URL: https://github.com/apache/accumulo/pull/2086#discussion_r630094180
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java
##########
@@ -144,27 +144,37 @@ private static void update(ServerContext context, Mutation m, KeyExtent extent)
public static void update(ServerContext context, ServiceLock zooLock, Mutation m,
KeyExtent extent) {
Writer t = extent.isMeta() ? getRootTable(context) : getMetadataTable(context);
- update(context, t, zooLock, m);
+ update(context, t, zooLock, m, extent);
}
public static void update(ServerContext context, Writer t, ServiceLock zooLock, Mutation m) {
+ update(context, t, zooLock, m, null);
+ }
+
+ public static void update(ServerContext context, Writer t, ServiceLock zooLock, Mutation m,
+ KeyExtent extent) {
Review comment:
Cool. Fixed.
--
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
[GitHub] [accumulo] ctubbsii merged pull request #2086: Add optional extent to metadata update failure logs
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #2086:
URL: https://github.com/apache/accumulo/pull/2086
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2086: Add optional extent to metadata update failure logs
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2086:
URL: https://github.com/apache/accumulo/pull/2086#discussion_r629659466
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java
##########
@@ -144,27 +144,37 @@ private static void update(ServerContext context, Mutation m, KeyExtent extent)
public static void update(ServerContext context, ServiceLock zooLock, Mutation m,
KeyExtent extent) {
Writer t = extent.isMeta() ? getRootTable(context) : getMetadataTable(context);
- update(context, t, zooLock, m);
+ update(context, t, zooLock, m, extent);
}
public static void update(ServerContext context, Writer t, ServiceLock zooLock, Mutation m) {
+ update(context, t, zooLock, m, null);
+ }
+
+ public static void update(ServerContext context, Writer t, ServiceLock zooLock, Mutation m,
+ KeyExtent extent) {
if (zooLock != null)
putLockID(context, zooLock, m);
while (true) {
try {
t.update(m);
return;
} catch (AccumuloException | TableNotFoundException | AccumuloSecurityException e) {
- log.error("{}", e.getMessage(), e);
+ logUpdateFailure(m, extent, e);
} catch (ConstraintViolationException e) {
- log.error("{}", e.getMessage(), e);
+ logUpdateFailure(m, extent, e);
// retrying when a CVE occurs is probably futile and can cause problems, see ACCUMULO-3096
throw new RuntimeException(e);
}
sleepUninterruptibly(1, TimeUnit.SECONDS);
}
}
+ private static void logUpdateFailure(Mutation m, KeyExtent extent, Exception e) {
+ String extentMsg = extent == null ? "" : " for extent: " + extent;
+ log.error("Failed to write metadata updates {} {}", extentMsg, m.prettyPrint(), e);
Review comment:
This suggestion is superseded by my subsequent suggestion to remove this ternary logic, since extent never needs to be null.
--
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
[GitHub] [accumulo] jschmidt10 commented on a change in pull request #2086: Add optional extent to metadata update failure logs
Posted by GitBox <gi...@apache.org>.
jschmidt10 commented on a change in pull request #2086:
URL: https://github.com/apache/accumulo/pull/2086#discussion_r629378428
##########
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:
Added since tag, rebased to main, and made the method an instance 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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2086: Add optional extent to metadata update failure logs
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2086:
URL: https://github.com/apache/accumulo/pull/2086#discussion_r629652242
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java
##########
@@ -144,27 +144,37 @@ private static void update(ServerContext context, Mutation m, KeyExtent extent)
public static void update(ServerContext context, ServiceLock zooLock, Mutation m,
KeyExtent extent) {
Writer t = extent.isMeta() ? getRootTable(context) : getMetadataTable(context);
- update(context, t, zooLock, m);
+ update(context, t, zooLock, m, extent);
}
public static void update(ServerContext context, Writer t, ServiceLock zooLock, Mutation m) {
+ update(context, t, zooLock, m, null);
+ }
+
+ public static void update(ServerContext context, Writer t, ServiceLock zooLock, Mutation m,
+ KeyExtent extent) {
if (zooLock != null)
putLockID(context, zooLock, m);
while (true) {
try {
t.update(m);
return;
} catch (AccumuloException | TableNotFoundException | AccumuloSecurityException e) {
- log.error("{}", e.getMessage(), e);
+ logUpdateFailure(m, extent, e);
} catch (ConstraintViolationException e) {
- log.error("{}", e.getMessage(), e);
+ logUpdateFailure(m, extent, e);
// retrying when a CVE occurs is probably futile and can cause problems, see ACCUMULO-3096
throw new RuntimeException(e);
}
sleepUninterruptibly(1, TimeUnit.SECONDS);
}
}
+ private static void logUpdateFailure(Mutation m, KeyExtent extent, Exception e) {
+ String extentMsg = extent == null ? "" : " for extent: " + extent;
+ log.error("Failed to write metadata updates {} {}", extentMsg, m.prettyPrint(), e);
Review comment:
Removes the second space after the word `updates`:
```suggestion
String extentMsg = extent == null ? "" : " for extent: " + extent;
log.error("Failed to write metadata updates{} {}", extentMsg, m.prettyPrint(), e);
```
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java
##########
@@ -144,27 +144,37 @@ private static void update(ServerContext context, Mutation m, KeyExtent extent)
public static void update(ServerContext context, ServiceLock zooLock, Mutation m,
KeyExtent extent) {
Writer t = extent.isMeta() ? getRootTable(context) : getMetadataTable(context);
- update(context, t, zooLock, m);
+ update(context, t, zooLock, m, extent);
}
public static void update(ServerContext context, Writer t, ServiceLock zooLock, Mutation m) {
+ update(context, t, zooLock, m, null);
+ }
+
+ public static void update(ServerContext context, Writer t, ServiceLock zooLock, Mutation m,
+ KeyExtent extent) {
Review comment:
I see there's only ever two places in the code that called the old method without the extent. The first is the one you changed above to call the new method, and where extent is never null. The second is in MetaConstraintRetryIT, which also has an extent that is non-null. I think there's no reason to have both update methods. You can just add extent to the existing one, without preserving the old behavior. You only have to update two references, and there's never a need to pass null into the method for extent, or to check for null in the log message where you have the ternary operator to change the method if it's null. If, for some reason it does end up being null, it's not a problem for it to say "for extent null".
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2086: Add optional extent to metadata update failure logs
Posted by 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
[GitHub] [accumulo] jschmidt10 commented on a change in pull request #2086: Add optional extent to metadata update failure logs
Posted by GitBox <gi...@apache.org>.
jschmidt10 commented on a change in pull request #2086:
URL: https://github.com/apache/accumulo/pull/2086#discussion_r629379224
##########
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:
Yes, the build removed that for me I think. Since I re-targeted main, this is gone (from this PR).
--
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