Revert "Revert "Jvenstad/fix node repo cache""

3 files changed, 81 insertions, 28 deletions

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 bf82128dfb7..149510bdc97 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
@@ -3,14 +3,17 @@ package com.yahoo.vespa.hosted.provision.persistence;
 
 import com.yahoo.path.Path;
 import com.yahoo.transaction.NestedTransaction;
+import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.curator.mock.MockCurator;
+import com.yahoo.vespa.curator.transaction.CuratorOperation;
 import com.yahoo.vespa.curator.transaction.CuratorOperations;
 import com.yahoo.vespa.curator.transaction.CuratorTransaction;
+import com.yahoo.vespa.curator.transaction.TransactionChanges;
 import org.junit.Test;
 
 import java.util.List;
-import java.util.Optional;
 
+import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.assertFalse;
@@ -26,7 +29,7 @@ import static org.junit.Assert.fail;
 public class CuratorDatabaseTest {
 
     @Test
-    public void testTransactionsIncreaseTimer() throws Exception {
+    public void testTransactionsIncreaseCounter() throws Exception {
         MockCurator curator = new MockCurator();
         CuratorDatabase database = new CuratorDatabase(curator, Path.fromString("/"), true);
 
@@ -36,9 +39,8 @@ public class CuratorDatabaseTest {
         commitCreate("/2", database);
         commitCreate("/1/1", database);
         commitCreate("/2/1", database);
-        assertEquals(4L, (long)curator.counter("/changeCounter").get().get().postValue());
+        assertEquals(8L, (long)curator.counter("/changeCounter").get().get().postValue());
 
-        List<String> children1Call0 = database.getChildren(Path.fromString("/1")); // prime the db; this call returns a different instance
         List<String> children1Call1 = database.getChildren(Path.fromString("/1"));
         List<String> children1Call2 = database.getChildren(Path.fromString("/1"));
         assertTrue("We reuse cached data when there are no commits", children1Call1 == children1Call2);
@@ -49,7 +51,24 @@ public class CuratorDatabaseTest {
         assertEquals(2, database.getChildren(Path.fromString("/2")).size());
         assertFalse("We do not reuse cached data in different parts of the tree when there are commits",
                     children1Call3 == children1Call2);
-        
+    }
+
+    @Test
+    public void testCacheInvalidation() throws Exception {
+        MockCurator curator = new MockCurator();
+        CuratorDatabase database = new CuratorDatabase(curator, Path.fromString("/"), true);
+
+        assertEquals(0L, (long)curator.counter("/changeCounter").get().get().postValue());
+        commitCreate("/1", database);
+        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(4L, (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
@@ -63,7 +82,7 @@ public class CuratorDatabaseTest {
         commitCreate("/2", database);
         commitCreate("/1/1", database);
         commitCreate("/2/1", database);
-        assertEquals(4L, (long)curator.counter("/changeCounter").get().get().postValue());
+        assertEquals(8L, (long)curator.counter("/changeCounter").get().get().postValue());
 
         List<String> children1Call0 = database.getChildren(Path.fromString("/1")); // prime the db; this call returns a different instance
         List<String> children1Call1 = database.getChildren(Path.fromString("/1"));
@@ -72,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);
 
@@ -85,32 +104,16 @@ public class CuratorDatabaseTest {
         catch (Exception expected) {
             // expected because the parent does not exist
         }
+        // Counter increased once, since prepare failed.
         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);
-
-        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
-        }
-        assertEquals(1L, (long)curator.counter("/changeCounter").get().get().postValue());
+        catch (Exception expected) { }
+        // Counter increased, even though commit failed.
+        assertEquals(3L, (long)curator.counter("/changeCounter").get().get().postValue());
     }
 
     private void commitCreate(String path, CuratorDatabase database) {
@@ -120,4 +123,41 @@ public class CuratorDatabaseTest {
         t.commit();
     }
 
+    private void commitReadingWrite(String path, byte[] data, CuratorDatabase database) {
+        NestedTransaction transaction = new NestedTransaction();
+        byte[] oldData = database.getData(Path.fromString(path)).get();
+        CuratorTransaction curatorTransaction = database.newCuratorTransactionIn(transaction);
+        // Add a dummy operation which reads the data and populates the cache during commit of the write.
+        curatorTransaction.add(new DummyOperation(() -> assertArrayEquals(oldData, database.getData(Path.fromString(path)).get())));
+        curatorTransaction.add(CuratorOperations.setData(path, data));
+        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;
+
+        public DummyOperation(Runnable task) {
+            this.task = task;
+        }
+
+        @Override
+        public org.apache.curator.framework.api.transaction.CuratorTransaction and(org.apache.curator.framework.api.transaction.CuratorTransaction transaction) {
+            task.run();
+            return transaction;
+        }
+
+        @Override
+        public void check(Curator curator, TransactionChanges changes) { }
+
+    }
+
 }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/flags1.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/flags1.json
index 8fd09b4a274..f27545f6094 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/flags1.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/flags1.json
@@ -5,6 +5,13 @@
       "enabled": false,
       "enabledHostnames": [],
       "enabledApplications": []
+    },
+    {
+      "id": "new-cache-counting",
+      "enabled": false,
+      "enabledHostnames": [],
+      "enabledApplications": []
     }
+
   ]
 }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/flags2.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/flags2.json
index 78de52e4e85..a0e9954bec4 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/flags2.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/flags2.json
@@ -9,6 +9,12 @@
       "enabledApplications": [
         "foo:bar:default"
       ]
+    },
+    {
+      "id": "new-cache-counting",
+      "enabled": false,
+      "enabledHostnames": [],
+      "enabledApplications": []
     }
   ]
 }