keith-turner commented on code in PR #4282:
URL: https://github.com/apache/accumulo/pull/4282#discussion_r1497669090
##########
test/src/main/java/org/apache/accumulo/test/fate/zookeeper/FateIT.java:
##########
@@ -369,6 +429,69 @@ public void testCancelWhileInCall() throws Exception {
}
+ @Test
+ public void testRepoFails() throws Exception {
+ /*
+ * This test ensures that when an exception occurs in a Repo's call() or
isReady() methods, that
+ * undo() will be called back up the chain of Repo's and in the correct
order. The test works as
+ * follows: 1) Repo1 is called and returns Repo2, 2) Repo2 is called and
returns Repo3, 3) Repo3
+ * is called and throws an exception (in call() or isReady()). It is then
expected that: 1)
+ * undo() is called on Repo3, 2) undo() is called on Repo2, 3) undo() is
called on Repo1
+ */
+ final ZooStore<Manager> zooStore = new ZooStore<Manager>(ZK_ROOT +
Constants.ZFATE, zk);
+ final AgeOffStore<Manager> store =
+ new AgeOffStore<Manager>(zooStore, 3000, System::currentTimeMillis);
+
+ Manager manager = createMock(Manager.class);
+ ServerContext sctx = createMock(ServerContext.class);
+ expect(manager.getContext()).andReturn(sctx).anyTimes();
+ expect(sctx.getZooKeeperRoot()).andReturn(ZK_ROOT).anyTimes();
+ expect(sctx.getZooReaderWriter()).andReturn(zk).anyTimes();
+ replay(manager, sctx);
+
+ Fate<Manager> fate = new Fate<Manager>(manager, store,
TraceRepo::toLogString);
+ try {
+ ConfigurationCopy config = new ConfigurationCopy();
+ config.set(Property.GENERAL_THREADPOOL_SIZE, "2");
+ config.set(Property.MANAGER_FATE_THREADPOOL_SIZE, "1");
+ fate.startTransactionRunners(config);
+
+ // Wait for the transaction runner to be scheduled.
+ UtilWaitThread.sleep(3000);
+
+ List<String> expectedUndoOrder = List.of("OP3", "OP2", "OP1");
+ /*
+ * Test exception in call()
+ */
+ undoLatch = new CountDownLatch(TestOperationFails.TOTAL_NUM_OPS);
+ long txid = fate.startTransaction();
+ assertEquals(NEW, getTxStatus(zk, txid));
+ fate.seedTransaction("TestOperationFails", txid,
+ new TestOperationFails(1, ExceptionLocation.CALL), true, "Test Op
Fails");
+ assertEquals(SUBMITTED, getTxStatus(zk, txid));
+ // Wait for all the undo() calls to complete
+ undoLatch.await();
+ assertEquals(expectedUndoOrder, TestOperationFails.undoOrder);
+ assertEquals(FAILED_IN_PROGRESS, getTxStatus(zk, txid));
+ /*
+ * Test exception in isReady()
+ */
+ TestOperationFails.undoOrder = new ArrayList<>();
+ undoLatch = new CountDownLatch(TestOperationFails.TOTAL_NUM_OPS);
+ txid = fate.startTransaction();
+ assertEquals(NEW, getTxStatus(zk, txid));
+ fate.seedTransaction("TestOperationFails", txid,
+ new TestOperationFails(1, ExceptionLocation.IS_READY), true, "Test
Op Fails");
+ assertEquals(SUBMITTED, getTxStatus(zk, txid));
Review Comment:
There is a race condition with this check, if a sleep is added and the test
run will probably see it. The status could change from SUBMITTED at any time
as background fate threads start working on the seeded transaction.
```suggestion
Thread.sleep(1000);
assertEquals(SUBMITTED, getTxStatus(zk, txid));
```
##########
test/src/main/java/org/apache/accumulo/test/fate/zookeeper/FateIT.java:
##########
@@ -369,6 +429,69 @@ public void testCancelWhileInCall() throws Exception {
}
+ @Test
+ public void testRepoFails() throws Exception {
+ /*
+ * This test ensures that when an exception occurs in a Repo's call() or
isReady() methods, that
+ * undo() will be called back up the chain of Repo's and in the correct
order. The test works as
+ * follows: 1) Repo1 is called and returns Repo2, 2) Repo2 is called and
returns Repo3, 3) Repo3
+ * is called and throws an exception (in call() or isReady()). It is then
expected that: 1)
+ * undo() is called on Repo3, 2) undo() is called on Repo2, 3) undo() is
called on Repo1
+ */
+ final ZooStore<Manager> zooStore = new ZooStore<Manager>(ZK_ROOT +
Constants.ZFATE, zk);
+ final AgeOffStore<Manager> store =
+ new AgeOffStore<Manager>(zooStore, 3000, System::currentTimeMillis);
+
+ Manager manager = createMock(Manager.class);
+ ServerContext sctx = createMock(ServerContext.class);
+ expect(manager.getContext()).andReturn(sctx).anyTimes();
+ expect(sctx.getZooKeeperRoot()).andReturn(ZK_ROOT).anyTimes();
+ expect(sctx.getZooReaderWriter()).andReturn(zk).anyTimes();
+ replay(manager, sctx);
+
+ Fate<Manager> fate = new Fate<Manager>(manager, store,
TraceRepo::toLogString);
+ try {
+ ConfigurationCopy config = new ConfigurationCopy();
+ config.set(Property.GENERAL_THREADPOOL_SIZE, "2");
+ config.set(Property.MANAGER_FATE_THREADPOOL_SIZE, "1");
+ fate.startTransactionRunners(config);
+
+ // Wait for the transaction runner to be scheduled.
+ UtilWaitThread.sleep(3000);
+
+ List<String> expectedUndoOrder = List.of("OP3", "OP2", "OP1");
+ /*
+ * Test exception in call()
+ */
+ undoLatch = new CountDownLatch(TestOperationFails.TOTAL_NUM_OPS);
+ long txid = fate.startTransaction();
+ assertEquals(NEW, getTxStatus(zk, txid));
+ fate.seedTransaction("TestOperationFails", txid,
+ new TestOperationFails(1, ExceptionLocation.CALL), true, "Test Op
Fails");
+ assertEquals(SUBMITTED, getTxStatus(zk, txid));
+ // Wait for all the undo() calls to complete
+ undoLatch.await();
+ assertEquals(expectedUndoOrder, TestOperationFails.undoOrder);
+ assertEquals(FAILED_IN_PROGRESS, getTxStatus(zk, txid));
+ /*
+ * Test exception in isReady()
+ */
+ TestOperationFails.undoOrder = new ArrayList<>();
+ undoLatch = new CountDownLatch(TestOperationFails.TOTAL_NUM_OPS);
+ txid = fate.startTransaction();
+ assertEquals(NEW, getTxStatus(zk, txid));
+ fate.seedTransaction("TestOperationFails", txid,
+ new TestOperationFails(1, ExceptionLocation.IS_READY), true, "Test
Op Fails");
+ assertEquals(SUBMITTED, getTxStatus(zk, txid));
+ // Wait for all the undo() calls to complete
+ undoLatch.await();
+ assertEquals(expectedUndoOrder, TestOperationFails.undoOrder);
+ assertEquals(FAILED_IN_PROGRESS, getTxStatus(zk, txid));
Review Comment:
There is a race condition with this check, could do the following which
should not have a race condition and utilizes and verifies more of the Fate
API. To do the following would need to disable auto cleanup, will make another
comment about that.
```suggestion
assertEquals(FAILED, fate.waitForCompletion(txid));
assertTrue(fate.getException(txid).getMessage().contains("isReady()
failed"));
```
##########
test/src/main/java/org/apache/accumulo/test/fate/zookeeper/FateIT.java:
##########
@@ -369,6 +429,69 @@ public void testCancelWhileInCall() throws Exception {
}
+ @Test
+ public void testRepoFails() throws Exception {
+ /*
+ * This test ensures that when an exception occurs in a Repo's call() or
isReady() methods, that
+ * undo() will be called back up the chain of Repo's and in the correct
order. The test works as
+ * follows: 1) Repo1 is called and returns Repo2, 2) Repo2 is called and
returns Repo3, 3) Repo3
+ * is called and throws an exception (in call() or isReady()). It is then
expected that: 1)
+ * undo() is called on Repo3, 2) undo() is called on Repo2, 3) undo() is
called on Repo1
+ */
+ final ZooStore<Manager> zooStore = new ZooStore<Manager>(ZK_ROOT +
Constants.ZFATE, zk);
+ final AgeOffStore<Manager> store =
+ new AgeOffStore<Manager>(zooStore, 3000, System::currentTimeMillis);
+
+ Manager manager = createMock(Manager.class);
+ ServerContext sctx = createMock(ServerContext.class);
+ expect(manager.getContext()).andReturn(sctx).anyTimes();
+ expect(sctx.getZooKeeperRoot()).andReturn(ZK_ROOT).anyTimes();
+ expect(sctx.getZooReaderWriter()).andReturn(zk).anyTimes();
+ replay(manager, sctx);
+
+ Fate<Manager> fate = new Fate<Manager>(manager, store,
TraceRepo::toLogString);
+ try {
+ ConfigurationCopy config = new ConfigurationCopy();
+ config.set(Property.GENERAL_THREADPOOL_SIZE, "2");
+ config.set(Property.MANAGER_FATE_THREADPOOL_SIZE, "1");
+ fate.startTransactionRunners(config);
+
+ // Wait for the transaction runner to be scheduled.
+ UtilWaitThread.sleep(3000);
+
+ List<String> expectedUndoOrder = List.of("OP3", "OP2", "OP1");
+ /*
+ * Test exception in call()
+ */
+ undoLatch = new CountDownLatch(TestOperationFails.TOTAL_NUM_OPS);
+ long txid = fate.startTransaction();
+ assertEquals(NEW, getTxStatus(zk, txid));
+ fate.seedTransaction("TestOperationFails", txid,
+ new TestOperationFails(1, ExceptionLocation.CALL), true, "Test Op
Fails");
+ assertEquals(SUBMITTED, getTxStatus(zk, txid));
+ // Wait for all the undo() calls to complete
+ undoLatch.await();
+ assertEquals(expectedUndoOrder, TestOperationFails.undoOrder);
+ assertEquals(FAILED_IN_PROGRESS, getTxStatus(zk, txid));
+ /*
+ * Test exception in isReady()
+ */
+ TestOperationFails.undoOrder = new ArrayList<>();
+ undoLatch = new CountDownLatch(TestOperationFails.TOTAL_NUM_OPS);
+ txid = fate.startTransaction();
+ assertEquals(NEW, getTxStatus(zk, txid));
+ fate.seedTransaction("TestOperationFails", txid,
+ new TestOperationFails(1, ExceptionLocation.IS_READY), true, "Test
Op Fails");
Review Comment:
Disabling auto cleanup makes waitForCompletion suggestion in another comment
work.
```suggestion
new TestOperationFails(1, ExceptionLocation.IS_READY), false,
"Test Op Fails");
```
##########
test/src/main/java/org/apache/accumulo/test/fate/zookeeper/FateIT.java:
##########
@@ -369,6 +429,69 @@ public void testCancelWhileInCall() throws Exception {
}
+ @Test
+ public void testRepoFails() throws Exception {
+ /*
+ * This test ensures that when an exception occurs in a Repo's call() or
isReady() methods, that
+ * undo() will be called back up the chain of Repo's and in the correct
order. The test works as
+ * follows: 1) Repo1 is called and returns Repo2, 2) Repo2 is called and
returns Repo3, 3) Repo3
+ * is called and throws an exception (in call() or isReady()). It is then
expected that: 1)
+ * undo() is called on Repo3, 2) undo() is called on Repo2, 3) undo() is
called on Repo1
+ */
+ final ZooStore<Manager> zooStore = new ZooStore<Manager>(ZK_ROOT +
Constants.ZFATE, zk);
+ final AgeOffStore<Manager> store =
+ new AgeOffStore<Manager>(zooStore, 3000, System::currentTimeMillis);
+
+ Manager manager = createMock(Manager.class);
+ ServerContext sctx = createMock(ServerContext.class);
+ expect(manager.getContext()).andReturn(sctx).anyTimes();
+ expect(sctx.getZooKeeperRoot()).andReturn(ZK_ROOT).anyTimes();
+ expect(sctx.getZooReaderWriter()).andReturn(zk).anyTimes();
+ replay(manager, sctx);
+
+ Fate<Manager> fate = new Fate<Manager>(manager, store,
TraceRepo::toLogString);
+ try {
+ ConfigurationCopy config = new ConfigurationCopy();
+ config.set(Property.GENERAL_THREADPOOL_SIZE, "2");
+ config.set(Property.MANAGER_FATE_THREADPOOL_SIZE, "1");
+ fate.startTransactionRunners(config);
+
+ // Wait for the transaction runner to be scheduled.
+ UtilWaitThread.sleep(3000);
+
+ List<String> expectedUndoOrder = List.of("OP3", "OP2", "OP1");
+ /*
+ * Test exception in call()
+ */
+ undoLatch = new CountDownLatch(TestOperationFails.TOTAL_NUM_OPS);
+ long txid = fate.startTransaction();
+ assertEquals(NEW, getTxStatus(zk, txid));
+ fate.seedTransaction("TestOperationFails", txid,
+ new TestOperationFails(1, ExceptionLocation.CALL), true, "Test Op
Fails");
+ assertEquals(SUBMITTED, getTxStatus(zk, txid));
+ // Wait for all the undo() calls to complete
+ undoLatch.await();
+ assertEquals(expectedUndoOrder, TestOperationFails.undoOrder);
+ assertEquals(FAILED_IN_PROGRESS, getTxStatus(zk, txid));
Review Comment:
```suggestion
assertEquals(FAILED, fate.waitForCompletion(txid));
assertTrue(fate.getException(txid).getMessage().contains("call()
failed"));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
