You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "kevinrr888 (via GitHub)" <gi...@apache.org> on 2024/02/20 18:37:22 UTC
[PR] FateIT Repo Error Test [accumulo]
kevinrr888 opened a new pull request, #4282:
URL: https://github.com/apache/accumulo/pull/4282
Closes #4266
This adds a test `testRepoFails()` to `FateIT` in 2.1 which ensures `undo()` is called and in the expected order when an error occurs in a Repo's `call()` or `isReady()` methods.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FateIT Repo Error Test [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on PR #4282:
URL: https://github.com/apache/accumulo/pull/4282#issuecomment-1957758493
- Removed `SUBMITTED` check in the new test `testRepoFails` (not needed -
tested elsewhere)
- Changed tests `testTransactionStatus` and `testCancelWhileInCall` to start
the transaction runners after the `SUBMITTED` check (the race condition
existed in these tests too)
- Resolved race condition on checking the `FAILED` state of the
transaction in `testRepoFails`
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FateIT Repo Error Test [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FateIT Repo Error Test [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #4282:
URL: https://github.com/apache/accumulo/pull/4282
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org