From e71f72757dd77f7bd6b0412e557274155bdc9e62 Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Wed, 6 Jul 2016 12:50:02 +0200 Subject: Towards transactional application delete --- .../com/yahoo/vespa/curator/mock/MockCurator.java | 1 + .../transaction/CuratorCreateOperation.java | 6 ++- .../transaction/CuratorDeleteOperation.java | 13 +++++- .../curator/transaction/CuratorOperation.java | 22 ++++++++--- .../transaction/CuratorSetDataOperation.java | 4 +- .../curator/transaction/CuratorTransaction.java | 8 +++- .../curator/transaction/TransactionChanges.java | 46 ++++++++++++++++++++++ 7 files changed, 87 insertions(+), 13 deletions(-) create mode 100644 zkfacade/src/main/java/com/yahoo/vespa/curator/transaction/TransactionChanges.java (limited to 'zkfacade/src') 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 { private final Curator curator; + private boolean prepared = false; public CuratorTransaction(Curator curator) { this.curator = curator; @@ -27,13 +28,18 @@ public class CuratorTransaction extends AbstractTransaction { @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 createdPaths = new HashSet<>(); + + /** The set of absolute paths deleted by this */ + private final Set 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; + } + +} -- cgit v1.2.3