[GitHub] [accumulo] keith-turner commented on a change in pull request #2175: Make DeleteTable fate lock reentrant

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

keith-turner commented on a change in pull request #2175:
URL: https://github.com/apache/accumulo/pull/2175#discussion_r656628253



##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java
##########
@@ -43,8 +43,13 @@ public DeleteTable(NamespaceId namespaceId, TableId tableId) {
   @Override
   public long isReady(long tid, Manager env) throws Exception {
 
-    // Before attempting to delete the table, cancel any running user
-    // compactions.
+    // If we have the table write lock, then it's possible that we have
+    // already canceled the compactions and we can skip to deleting the tables.
+    if (Utils.hasNamespaceReadLock(env, namespaceId, tid)
+        && Utils.hasTableWriteLock(env, tableId, tid)) {
+      return 0;
+    }
+    // Cancel any running user compactions before deleting table
     if (Utils.reserveNamespace(env, namespaceId, tid, false, true, TableOperation.COMPACT_CANCEL)

Review comment:
       I just noticed that this will not wait when it can not get the read lock.  Another possible way to solve this problem is to add two new Repos that run before DeleteTable.  When isReady() returns a non-zero value, then the operation will be retired again later.
   
   ```java
   class PredeleteTable extends ManagerRepo {
        public long isReady(long tid, Manager env) {
              // attempt to get read lock to for cancel compactoin
              return Utils.reserveNamespace(env, namespaceId, tid, false, true, TableOperation.COMPACT_CANCEL) +
                  Utils.reserveTable(env, tableId, tid, false, true, TableOperation.COMPACT_CANCEL);
        }
   
       public Repo<Manager> call(long tid, Manager env) {
              CancelCompactions.mutateZooKeeper(tid, tableId, env);
              return PredeleteTableUnlock(...);
        }
   
     @Override
     public void undo(long tid, Manager env) {
              Utils.unreserveTable(env, tableId, tid, false);
              Utils.unreserveNamespace(env, namespaceId, tid, false);
     }
   }
   
   class PredeleteTableUnlock extends ManagerRepo {
        public long isReady(long tid, Manager env) {
              // attempt to get read lock to for cancel compactoin
              return 0;
        }
   
       public Repo<Manager> call(long tid, Manager env) {
              Utils.unreserveTable(env, tableId, tid, false);
              Utils.unreserveNamespace(env, namespaceId, tid, false);
              return DeleteTable(...);  // call the existing first fate op
        }
   }
   ```
   
   Also, I think this pattern may not require the hasLockMethods.  It also avoid problem w/ undo on delete table if the read lock is held and not the write lock.




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