[GitHub] [accumulo] ctubbsii commented on a change in pull request #1759: Wrap undo in try block

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

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



##########
File path: server/manager/src/main/java/org/apache/accumulo/master/tableOps/create/CreateTable.java
##########
@@ -72,6 +77,9 @@ public long isReady(long tid, Master environment) throws Exception {
     Utils.getIdLock().lock();
     try {
       String tName = tableInfo.getTableName();
+      if(tName.equals("ci")){
+        Thread.sleep(10000000);
+      }

Review comment:
       This needs to be removed. A user could have a table by this name.

##########
File path: server/manager/src/main/java/org/apache/accumulo/master/tableOps/create/FinishCreateTable.java
##########
@@ -62,17 +67,23 @@ public long isReady(long tid, Master environment) {
     env.getEventCoordinator().event("Created table %s ", tableInfo.getTableName());
 
     if (tableInfo.getInitialSplitSize() > 0) {
-      cleanupSplitFiles(env);
+      cleanupSplitFiles(tid, env);
     }
     return null;
   }
 
-  private void cleanupSplitFiles(Master env) throws IOException {
+  private void cleanupSplitFiles(long tid, Master env) throws IOException {
     // it is sufficient to delete from the parent, because both files are in the same directory, and
     // we want to delete the directory also
-    Path p = tableInfo.getSplitPath().getParent();
-    FileSystem fs = p.getFileSystem(env.getContext().getHadoopConf());
-    fs.delete(p, true);
+    try {
+      Path p = tableInfo.getSplitPath().getParent();
+      FileSystem fs = p.getFileSystem(env.getContext().getHadoopConf());
+      fs.delete(p, true);
+    } catch (NullPointerException | IOException e) {
+      var spdir = Optional.ofNullable(tableInfo).map(TableInfo::getSplitDirsPath).orElse(null);
+      log.error("{} Failed to cleanup splits file after table was created, split dir {} ",
+          FateTxId.formatTid(tid), spdir, e);
+    }

Review comment:
       I don't think it's possible for tableInfo to be null here, given how this class is constructed. And, if the split size is `>0`, it's also not possible for the splits file locations to be null. Also, I think we should focus just on handling exceptions pertaining to the specific cleanup task that we're performing. So, if these do happen to be null because of some serialization bug or whatever, that shouldn't be handled here, but handled by the framework if the exception is thrown from the method (if the framework isn't capable of handling exceptions thrown by this method, the interface shouldn't have `throws Exception` on the method, and if that's a problem, it is a separate issue that should be fixed in a separate PR to fix the FaTE framework itself).
   
   Also, the tx id is already in the path to the splits file, so it's possibly redundant to have that here (but I'm not opposed to keeping it).
   
   Also, we probably want to log the parent path (the one we're trying to delete recursively), rather than just one of the two file names.
   
   So, all told, I think something like this would be a little cleaner (with similar changes made in the other places):
   
   ```suggestion
       Path p = null;
       try {
         p = tableInfo.getSplitPath().getParent();
         FileSystem fs = p.getFileSystem(env.getContext().getHadoopConf());
         fs.delete(p, true);
       } catch (IOException e) {
         log.error("Table was created, but failed to clean up temporary splits files at {}", p, e);
       }
   ```




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