[GitHub] [accumulo] ctubbsii commented on a change in pull request #2178: Catch NoNodeException in CompactRange

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

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



##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactRange.java
##########
@@ -149,24 +149,27 @@ static void removeIterators(Manager environment, final long txid, TableId tableI
 
     ZooReaderWriter zoo = environment.getContext().getZooReaderWriter();
 
-    zoo.mutateExisting(zTablePath, currentValue -> {
-      String cvs = new String(currentValue, UTF_8);
-      String[] tokens = cvs.split(",");
-      long flushID = Long.parseLong(tokens[0]);
-
-      String txidString = String.format("%016x", txid);
+    try {
+      zoo.mutateExisting(zTablePath, currentValue -> {
+        String cvs = new String(currentValue, UTF_8);
+        String[] tokens = cvs.split(",");
+        long flushID = Long.parseLong(tokens[0]);
 
-      StringBuilder encodedIterators = new StringBuilder();
-      for (int i = 1; i < tokens.length; i++) {
-        if (tokens[i].startsWith(txidString))
-          continue;
-        encodedIterators.append(",");
-        encodedIterators.append(tokens[i]);
-      }
+        String txidString = String.format("%016x", txid);
 
-      return (Long.toString(flushID) + encodedIterators).getBytes(UTF_8);
-    });
+        StringBuilder encodedIterators = new StringBuilder();
+        for (int i = 1; i < tokens.length; i++) {
+          if (tokens[i].startsWith(txidString))
+            continue;
+          encodedIterators.append(",");
+          encodedIterators.append(tokens[i]);
+        }
 
+        return (Long.toString(flushID) + encodedIterators).getBytes(UTF_8);
+      });
+    } catch (NoNodeException ke) {
+      log.debug("Node for {} no longer exists.", tableId, ke);
+    }

Review comment:
       I'm not sure how it's possible for this to run after a table is deleted, unless the lock reservations are relying on ZooCache to check table existence and it's somehow out-of-date. See a related comment about compaction IDs in https://github.com/apache/accumulo/pull/2175#discussion_r657292064
   
   In any case, elsewhere in that same class, we `throw new AcceptableThriftTableOperationException`, in response to `NoNodeException`. Should we do that here as well? I'm not sure who is even catching and handling that exception type. But, it's probably worth tracking down to see if they should be made consistent, or at least adding a comment what's different here that warrants a different reaction to the same exception.




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