diff options
author | Jon Marius Venstad <jvenstad@yahoo-inc.com> | 2018-12-12 11:05:29 +0100 |
---|---|---|
committer | Jon Marius Venstad <jvenstad@yahoo-inc.com> | 2018-12-12 11:05:29 +0100 |
commit | 908e8a4fde9c306410a498aa9f9fd75e64fc3128 (patch) | |
tree | beefb13079fe72526c70011f912d5780882f9d0d /node-repository | |
parent | 33c947878030781851559df57a10bd3c859fa1fb (diff) |
Use a counter which increments in finally, if commiting
Diffstat (limited to 'node-repository')
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; |