Towards transactional application delete

7 files changed, 87 insertions, 13 deletions

diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/mock/MockCurator.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/mock/MockCurator.java
index cc5d9cc10fb..9d347f8be7f 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/mock/MockCurator.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/mock/MockCurator.java
@@ -168,6 +168,7 @@ public class MockCurator extends Curator {
         Node parent = root.getNode(Paths.get(path.getParentPath().toString()), false);
         if (parent == null) throw new KeeperException.NoNodeException(path.toString());
         Node node = parent.children().get(path.getName());
+        if (node == null) throw new KeeperException.NoNodeException(path.getName() + " under " + parent);
         if ( ! node.children().isEmpty() && ! deleteChildren)
             throw new KeeperException.NotEmptyException(path.toString());
         parent.remove(path.getName());
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorCreateOperation.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorCreateOperation.java
index 340815d60a1..30d22fa68bc 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorCreateOperation.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorCreateOperation.java
@@ -24,12 +24,14 @@ class CuratorCreateOperation implements CuratorOperation {
     }
 
     @Override
-    public void check(Curator curator) {
+    public void check(Curator curator, TransactionChanges changes) {
         int lastSlash = path.lastIndexOf("/");
         if (lastSlash < 0) return; // root; ok
         String parent = path.substring(0, lastSlash);
-        if (!parent.isEmpty() && ! curator.exists(Path.fromString(parent)) )
+        if ( ! parent.isEmpty() && ! curator.exists(Path.fromString(parent)) && ! changes.creates(parent)) {
             throw new IllegalStateException("Cannot perform " + this + ": Parent '" + parent + "' does not exist");
+        }
+        changes.addCreates(path);
     }
 
     @Override
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorDeleteOperation.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorDeleteOperation.java
index 5ff0d4a143f..1965cd51c3d 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorDeleteOperation.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorDeleteOperation.java
@@ -13,6 +13,9 @@ class CuratorDeleteOperation implements CuratorOperation {
 
     private final String path;
     private final boolean throwIfNotExist;
+    
+    /** False iff we positively know this path does not exist */
+    private boolean pathExists = true;
 
     CuratorDeleteOperation(String path, boolean throwIfNotExist) {
         this.path = path;
@@ -20,13 +23,19 @@ class CuratorDeleteOperation implements CuratorOperation {
     }
 
     @Override
-    public void check(Curator curator) {
-        if ( throwIfNotExist && ! curator.exists(Path.fromString(path)) )
+    public void check(Curator curator, TransactionChanges changes) {
+        // TODO: Check children
+        pathExists = curator.exists(Path.fromString(path)) || changes.creates(path);
+        if ( throwIfNotExist && ! pathExists)
             throw new IllegalStateException("Cannot perform " + this + ": Path does not exist");
+        if ( ! pathExists)
+            changes.addDeletes(path);
     }
 
     @Override
     public CuratorTransaction and(CuratorTransaction transaction) throws Exception {
+        System.out.println("path: " + path + ", exists: " + pathExists);
+        if ( ! throwIfNotExist && ! pathExists) return transaction; // this is a noop
         return transaction.delete().forPath(path).and();
     }
 
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorOperation.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorOperation.java
index f0a4f5f1d66..4a062582518 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorOperation.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorOperation.java
@@ -3,11 +3,8 @@ package com.yahoo.vespa.curator.transaction;
 
 import com.yahoo.transaction.Transaction;
 import com.yahoo.vespa.curator.Curator;
-import org.apache.curator.framework.CuratorFramework;
 import org.apache.curator.framework.api.transaction.CuratorTransaction;
 
-import java.util.Optional;
-
 /**
  * The ZooKeeper operations that we support doing transactional.
  *
@@ -17,7 +14,7 @@ import java.util.Optional;
 public interface CuratorOperation extends Transaction.Operation {
 
     /**
-     * Implementations must support adding this operation to a curator transaction.
+     * Returns the transaction resulting from combining this operation with the input transaction
      *
      * @param transaction {@link CuratorTransaction} to append this operation to.
      * @return the transaction, for chaining.
@@ -26,10 +23,23 @@ public interface CuratorOperation extends Transaction.Operation {
     CuratorTransaction and(CuratorTransaction transaction) throws Exception;
 
     /**
-     * Check if this operation can be performed without making any changes.
+     * Check if this operation can be performed by calling check(curator, new TransactionChanges()).
      *
      * @throws IllegalStateException if it cannot
      */
-    void check(Curator curator);
+    default void check(Curator curator) {
+        check(curator, new TransactionChanges());        
+    }
+
+    /**
+     * Check if this operation can be performed.
+     *
+     * @param curator the curator instance to check against
+     * @param changes changes which will be done prior to this operation as part of the same transaction.
+     *        Operations should use both this and the curator instance to check if they can be performed.
+     *        In addition, they are required to add the change(s) they will perform to the set of changes.
+     * @throws IllegalStateException if it cannot be performed
+     */
+    void check(Curator curator, TransactionChanges changes);
 
 }
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorSetDataOperation.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorSetDataOperation.java
index eddca50a69e..daf284cbdc6 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorSetDataOperation.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorSetDataOperation.java
@@ -22,8 +22,8 @@ class CuratorSetDataOperation implements CuratorOperation {
     }
 
     @Override
-    public void check(Curator curator) {
-        if ( ! curator.exists(Path.fromString(path)) )
+    public void check(Curator curator, TransactionChanges changes) {
+        if ( ! curator.exists(Path.fromString(path)) && ! changes.creates(path) )
             throw new IllegalStateException("Cannot perform " + this + ": Path does not exist");
     }
 
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorTransaction.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorTransaction.java
index aa7643fb7ae..5624f103210 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorTransaction.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/CuratorTransaction.java
@@ -13,6 +13,7 @@ import org.apache.curator.framework.api.transaction.CuratorTransactionFinal;
 public class CuratorTransaction extends AbstractTransaction<CuratorOperation> {
 
     private final Curator curator;
+    private boolean prepared = false;
 
     public CuratorTransaction(Curator curator) {
         this.curator = curator;
@@ -27,13 +28,18 @@ public class CuratorTransaction extends AbstractTransaction<CuratorOperation> {
 
     @Override
     public void prepare() {
+        TransactionChanges changes = new TransactionChanges();
         for (CuratorOperation operation : operations())
-            operation.check(curator);
+            operation.check(curator, changes);
+        prepared = true;
     }
 
+    /** Commits this transaction. If it is not already prepared this will prepare it first */
     @Override
     public void commit() {
         try {
+            if ( ! prepared)
+                prepare();
             org.apache.curator.framework.api.transaction.CuratorTransaction transaction = curator.framework().inTransaction();
             for (CuratorOperation operation : operations()) {
                 transaction = operation.and(transaction);
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/TransactionChanges.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/TransactionChanges.java
new file mode 100644
index 00000000000..4a370ce464b
--- /dev/null
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/TransactionChanges.java
@@ -0,0 +1,46 @@
+package com.yahoo.vespa.curator.transaction;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Records the set of changes which will happen as part of a transaction
+ * 
+ * @author bratseth
+ */
+public class TransactionChanges {
+    
+    /** The set of absolute paths created by this */
+    private final Set<String> createdPaths = new HashSet<>();
+
+    /** The set of absolute paths deleted by this */
+    private final Set<String> deletedPaths = new HashSet<>();
+    
+    /** Returns whether the changes include creating this absolute path */
+    public boolean creates(String path) {
+        return createdPaths.contains(path);
+    }
+    
+    /** Adds creation of an absolute path to the set of changes made by this */
+    public void addCreates(String path) {
+        deletedPaths.remove(path);
+        createdPaths.add(path);
+    }
+
+    /** Returns whether the changes include deleting this absolute path */
+    public boolean deletes(String path) {
+        return deletedPaths.contains(path);
+    }
+
+    /** Adds deletion of an absolute path to the set of changes made by this */
+    public void addDeletes(String path) {
+        createdPaths.remove(path);
+        deletedPaths.add(path);
+    }
+
+    @Override
+    public String toString() {
+        return "Transaction changes: CREATES " + createdPaths + " DELETES " + deletedPaths;
+    }
+    
+}