keith-turner commented on code in PR #5334:
URL: https://github.com/apache/accumulo/pull/5334#discussion_r1985675685


##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/CloneTable.java:
##########
@@ -34,23 +34,18 @@ public class CloneTable extends ManagerRepo {
   private static final long serialVersionUID = 1L;
   private final CloneInfo cloneInfo;
 
-  public CloneTable(String user, NamespaceId namespaceId, TableId srcTableId, 
String tableName,
-      Map<String,String> propertiesToSet, Set<String> propertiesToExclude, 
boolean keepOffline) {
-    cloneInfo = new CloneInfo();
-    cloneInfo.user = user;
-    cloneInfo.srcTableId = srcTableId;
-    cloneInfo.tableName = tableName;
-    cloneInfo.propertiesToExclude = propertiesToExclude;
-    cloneInfo.propertiesToSet = propertiesToSet;
-    cloneInfo.srcNamespaceId = namespaceId;
-    cloneInfo.keepOffline = keepOffline;
+  public CloneTable(String user, NamespaceId srcNamespaceId, TableId 
srcTableId,
+      NamespaceId namespaceId, String tableName, Map<String,String> 
propertiesToSet,
+      Set<String> propertiesToExclude, boolean keepOffline) {
+    cloneInfo = new CloneInfo(srcNamespaceId, srcTableId, namespaceId, 
tableName, propertiesToSet,
+        propertiesToExclude, keepOffline, user);
   }
 
   @Override
   public long isReady(long tid, Manager environment) throws Exception {
-    long val = Utils.reserveNamespace(environment, cloneInfo.srcNamespaceId, 
tid, false, true,
+    long val = Utils.reserveNamespace(environment, cloneInfo.getNamespaceId(), 
tid, false, true,
         TableOperation.CLONE);
-    val += Utils.reserveTable(environment, cloneInfo.srcTableId, tid, false, 
true,
+    val += Utils.reserveTable(environment, cloneInfo.getSrcTableId(), tid, 
false, true,

Review Comment:
   We could try to optimize the locking via transitive assumptions I suppose.  
Like clone table can  assume delete table will get a read lock on the namesapce 
and a write lock on the table and therefore it does not need to get the 
namespace lock only the table read lock.  None of the fate operations work like 
this currently.  They just get the read lock on the namespace and then the 
table lock.  To me its much easier to reason about if all table ops follow the 
same protocol.



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

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to