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:
[email protected]
