Use a counter which increments in finally, if commiting

3 files changed, 74 insertions, 25 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CountingCuratorTransaction.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CountingCuratorTransaction.java
index 3db2f9dc902..629ada46852 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CountingCuratorTransaction.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CountingCuratorTransaction.java
@@ -1,5 +1,55 @@
 package com.yahoo.vespa.hosted.provision.persistence;
 
-public class CoutingCuratorTransaction {
+import com.yahoo.path.Path;
+import com.yahoo.vespa.curator.Curator;
+import com.yahoo.vespa.curator.Lock;
+import com.yahoo.vespa.curator.recipes.CuratorCounter;
+import com.yahoo.vespa.curator.transaction.CuratorOperation;
+import com.yahoo.vespa.curator.transaction.CuratorTransaction;
+import com.yahoo.vespa.curator.transaction.TransactionChanges;
+
+/**
+ * CuratorTransaction wrapper which increments a counter, to signal invalidation of node repository caches.
+ *
+ * This class ensures a CuratorTransaction against the cached data (the node repository data) is
+ * accompanied by an increment of the data generation counter. This increment must occur <em>after</em>
+ * the write has completed, successfully or not. It is therefore placed in a {@code finally} block,
+ * wrapping the super class' {@link #commit()}.
+ * The cache is invalid whenever the generation counter is higher than what the cache contents were read with.
+ * The usual locking for modifications of shared data is then enough to ensure the cache provides a
+ * consistent view of the shared data, with one exception: when incrementing the counter fails. This is
+ * assumed to be extremely rare, and the consequence is temporary neglect of cache invalidation.
+ *
+ * @author jonmv
+ */
+class CountingCuratorTransaction extends CuratorTransaction {
+
+    private final CuratorCounter counter;
+
+    public CountingCuratorTransaction(Curator curator, CuratorCounter counter) {
+        super(curator);
+        this.counter = counter;
+    }
+
+    @Override
+    public void prepare() {
+        counter.get();
+        super.prepare();
+    }
+
+    @Override
+    public void commit() {
+        try {
+            super.commit();
+        }
+        finally {
+            counter.next();
+        }
+    }
+
+    @Override
+    public String toString() {
+        return "(" + super.toString() + "), INCREMENT " + counter;
+    }
 
 }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java
index d8c1323151d..778d2c26866 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java
@@ -86,9 +86,8 @@ public class CuratorDatabase {
      * Important: It is the nested transaction which must be committed - never the curator transaction directly.
      */
     public CuratorTransaction newCuratorTransactionIn(NestedTransaction transaction) {
-        // Add a counting transaction first, to make sure we always invalidate the current state on any transaction commit
-        transaction.add(new EagerCountingCuratorTransaction(changeGenerationCounter), CuratorTransaction.class);
-        CuratorTransaction curatorTransaction = new CuratorTransaction(curator);
+        // Wrap the curator transaction with an increment of the generation counter.
+        CountingCuratorTransaction curatorTransaction = new CountingCuratorTransaction(curator, changeGenerationCounter);
         transaction.add(curatorTransaction);
         return curatorTransaction;
     }
@@ -97,6 +96,7 @@ public class CuratorDatabase {
     // As this operation does not depend on the prior state we do not need to increment the write counter
     public void create(Path path) {
         curator.create(path);
+        changeGenerationCounter.next(); // Increment counter to ensure getChildren sees any change.
     }
 
     /** Returns whether given path exists */
@@ -106,6 +106,8 @@ public class CuratorDatabase {
 
     // --------- Read operations -------------------------------------------------------------------------------
     // These can read from the memory file system, which accurately mirrors the ZooKeeper content IF
+    // the current generation counter is the same as it was when data was put into the cache, AND
+    // the data to read is protected by a lock which is held now, and during any writes of the data.
 
     /** Returns the immediate, local names of the children under this node in any order */
     public List<String> getChildren(Path path) { return getCache().getChildren(path); }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseTest.java
index f556cbeb8ca..835ac4d0913 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseTest.java
@@ -63,7 +63,12 @@ public class CuratorDatabaseTest {
         assertArrayEquals(new byte[0], database.getData(Path.fromString("/1")).get());
         commitReadingWrite("/1", "hello".getBytes(), database);
         // Data cached during commit of write transaction. Should be invalid now, and re-read.
+        assertEquals(2L, (long)curator.counter("/changeCounter").get().get().postValue());
         assertArrayEquals("hello".getBytes(), database.getData(Path.fromString("/1")).get());
+
+        assertEquals(0, database.getChildren(Path.fromString("/1")).size());
+        commitCreate("/1/1", database);
+        assertEquals(1, database.getChildren(Path.fromString("/1")).size());
     }
 
     @Test
@@ -86,7 +91,7 @@ public class CuratorDatabaseTest {
     }
 
     @Test
-    public void testThatCounterIncreasesAlsoOnCommitFailure() throws Exception {
+    public void testThatCounterIncreasesExactlyOnCommitFailure() throws Exception {
         MockCurator curator = new MockCurator();
         CuratorDatabase database = new CuratorDatabase(curator, Path.fromString("/"), true);
 
@@ -99,31 +104,15 @@ public class CuratorDatabaseTest {
         catch (Exception expected) {
             // expected because the parent does not exist
         }
-        assertEquals(1L, (long)curator.counter("/changeCounter").get().get().postValue());
-    }
-
-    @Test
-    public void testThatCounterIncreasesAlsoOnCommitFailureFromExistingTransaction() throws Exception {
-        MockCurator curator = new MockCurator();
-        CuratorDatabase database = new CuratorDatabase(curator, Path.fromString("/"), true);
-
+        // Counter not increased, since prepare failed.
         assertEquals(0L, (long)curator.counter("/changeCounter").get().get().postValue());
 
         try {
-            NestedTransaction t = new NestedTransaction();
-            CuratorTransaction separateC = new CuratorTransaction(curator);
-            separateC.add(CuratorOperations.create("/1/2")); // fail as parent does not exist
-            t.add(separateC);
-
-            CuratorTransaction c = database.newCuratorTransactionIn(t);
-            c.add(CuratorOperations.create("/1")); // does not fail
-
-            t.commit();
+            commitFailing(database); // fail during commit
             fail("Expected exception");
         }
-        catch (Exception expected) {
-            // expected because the parent does not exist
-        }
+        catch (Exception expected) { }
+        // Counter increased, since commit failed.
         assertEquals(1L, (long)curator.counter("/changeCounter").get().get().postValue());
     }
 
@@ -144,6 +133,14 @@ public class CuratorDatabaseTest {
         transaction.commit();
     }
 
+    /** Commit an operation which fails during commit. */
+    private void commitFailing(CuratorDatabase database) {
+        NestedTransaction t = new NestedTransaction();
+        CuratorTransaction c = database.newCuratorTransactionIn(t);
+        c.add(new DummyOperation(() -> { throw new RuntimeException(); }));
+        t.commit();
+    }
+
     static class DummyOperation implements CuratorOperation {
 
         private final Runnable task;