Fix GEODE-186 by removing sleeps in tests The old test scheduled tx suspension to timeout after 1 minute. So the test always run for at least 1 minute. A test hook now exists that allows the test to specify a different time unit (default is still minutes) for tx suspension expiration.
The sleeps in a bunch of other tests were not needed since the tx operation is synchronous. So those sleeps have simply been removed. A couple of sleeps in clients waiting for something to arrive that was done on a server have been converted to a wait since server to client distribution is async. Reviewed by Swapnil. Project: http://git-wip-us.apache.org/repos/asf/incubator-geode/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-geode/commit/389e2a8c Tree: http://git-wip-us.apache.org/repos/asf/incubator-geode/tree/389e2a8c Diff: http://git-wip-us.apache.org/repos/asf/incubator-geode/diff/389e2a8c Branch: refs/heads/feature/GEODE-137 Commit: 389e2a8cac1f2b35aa3ebfd10e8c49ae993b0dc4 Parents: e4c77c6 Author: Darrel Schneider <dschnei...@pivotal.io> Authored: Tue Aug 4 11:43:39 2015 -0700 Committer: Qihong Chen <qc...@pivotal.io> Committed: Thu Aug 6 10:07:48 2015 -0700 ---------------------------------------------------------------------- .../gemfire/internal/cache/TXManagerImpl.java | 11 +++++-- .../cache/ClientServerTransactionDUnitTest.java | 32 ++++++++++---------- .../cache/RemoteTransactionDUnitTest.java | 30 ++++++++++++------ 3 files changed, 45 insertions(+), 28 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/389e2a8c/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/TXManagerImpl.java ---------------------------------------------------------------------- diff --git a/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/TXManagerImpl.java b/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/TXManagerImpl.java index 88714b0..dde3793 100644 --- a/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/TXManagerImpl.java +++ b/gemfire-core/src/main/java/com/gemstone/gemfire/internal/cache/TXManagerImpl.java @@ -1117,6 +1117,10 @@ public final class TXManagerImpl implements CacheTransactionManager, private ConcurrentMap<TransactionId, TXStateProxy> suspendedTXs = new ConcurrentHashMap<TransactionId, TXStateProxy>(); public TransactionId suspend() { + return suspend(TimeUnit.MINUTES); + } + + TransactionId suspend(TimeUnit expiryTimeUnit) { TXStateProxy result = getTXState(); if (result != null) { TransactionId txId = result.getTransactionId(); @@ -1137,7 +1141,7 @@ public final class TXManagerImpl implements CacheTransactionManager, LockSupport.unpark(waitingThread); } } - scheduleExpiry(txId); + scheduleExpiry(txId, expiryTimeUnit); return txId; } return null; @@ -1266,8 +1270,9 @@ public final class TXManagerImpl implements CacheTransactionManager, /** * schedules the transaction to expire after {@link #suspendedTXTimeout} * @param txId + * @param expiryTimeUnit the time unit to use when scheduling the expiration */ - private void scheduleExpiry(TransactionId txId) { + private void scheduleExpiry(TransactionId txId, TimeUnit expiryTimeUnit) { final GemFireCacheImpl cache = (GemFireCacheImpl) this.cache; if (suspendedTXTimeout < 0) { if (logger.isDebugEnabled()) { @@ -1279,7 +1284,7 @@ public final class TXManagerImpl implements CacheTransactionManager, if (logger.isDebugEnabled()) { logger.debug("TX: scheduling transaction: {} to expire after:{}", txId, suspendedTXTimeout); } - cache.getCCPTimer().schedule(task, suspendedTXTimeout*60*1000); + cache.getCCPTimer().schedule(task, TimeUnit.MILLISECONDS.convert(suspendedTXTimeout, expiryTimeUnit)); this.expiryTasks.put(txId, task); } http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/389e2a8c/gemfire-core/src/test/java/com/gemstone/gemfire/internal/cache/ClientServerTransactionDUnitTest.java ---------------------------------------------------------------------- diff --git a/gemfire-core/src/test/java/com/gemstone/gemfire/internal/cache/ClientServerTransactionDUnitTest.java b/gemfire-core/src/test/java/com/gemstone/gemfire/internal/cache/ClientServerTransactionDUnitTest.java index 3ce8cec..d80f6bb 100644 --- a/gemfire-core/src/test/java/com/gemstone/gemfire/internal/cache/ClientServerTransactionDUnitTest.java +++ b/gemfire-core/src/test/java/com/gemstone/gemfire/internal/cache/ClientServerTransactionDUnitTest.java @@ -16,6 +16,7 @@ import java.util.Map; import java.util.Properties; import java.util.Set; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import javax.naming.Context; import javax.transaction.UserTransaction; @@ -78,6 +79,7 @@ import com.gemstone.gemfire.internal.cache.execute.util.CommitFunction; import com.gemstone.gemfire.internal.cache.execute.util.RollbackFunction; import com.gemstone.gemfire.internal.cache.tx.ClientTXStateStub; +import dunit.DistributedTestCase; import dunit.Host; import dunit.SerializableCallable; import dunit.SerializableRunnable; @@ -738,7 +740,6 @@ public void testClientCommitAndDataStoreGetsEvent() throws Exception { } }); - Thread.sleep(10000); datastore.invoke(new SerializableCallable() { public Object call() throws Exception { Region<CustId, Customer> custRegion = getCache().getRegion(CUSTOMER); @@ -926,14 +927,12 @@ public void testClientCommitAndDataStoreGetsEvent() throws Exception { } }); - Thread.sleep(10000); client.invoke(new SerializableCallable() { public Object call() throws Exception { Region<CustId, Customer> custRegion = getCache().getRegion(CUSTOMER); // Region<OrderId, Order> orderRegion = getCache().getRegion(ORDER); // Region<CustId,Customer> refRegion = getCache().getRegion(D_REFERENCE); ClientListener cl = (ClientListener) custRegion.getAttributes().getCacheListeners()[0]; - getCache().getLogger().info("SWAP:CLIENTinvoked:"+cl.invoked); assertTrue(cl.invoked); assertEquals("it should be 1 but its:"+cl.invokeCount,1,cl.invokeCount); return null; @@ -981,14 +980,12 @@ public void testClientCommitAndDataStoreGetsEvent() throws Exception { } }); - Thread.sleep(10000); client.invoke(new SerializableCallable() { public Object call() throws Exception { Region<CustId, Customer> custRegion = getCache().getRegion(CUSTOMER); // Region<OrderId, Order> orderRegion = getCache().getRegion(ORDER); // Region<CustId,Customer> refRegion = getCache().getRegion(D_REFERENCE); ClientListener cl = (ClientListener) custRegion.getAttributes().getCacheListeners()[0]; - getCache().getLogger().info("SWAP:CLIENTinvoked:"+cl.invoked); assertTrue(cl.invoked); assertEquals("it should be 1 but its:"+cl.invokeCount,1,cl.invokeCount); return null; @@ -1038,14 +1035,12 @@ public void testClientCommitAndDataStoreGetsEvent() throws Exception { } }); - Thread.sleep(10000); client.invoke(new SerializableCallable() { public Object call() throws Exception { Region<CustId, Customer> custRegion = getCache().getRegion(CUSTOMER); // Region<OrderId, Order> orderRegion = getCache().getRegion(ORDER); // Region<CustId,Customer> refRegion = getCache().getRegion(D_REFERENCE); ClientListener cl = (ClientListener) custRegion.getAttributes().getCacheListeners()[0]; - getCache().getLogger().info("SWAP:CLIENTinvoked:"+cl.invoked); assertTrue(cl.invoked); assertTrue(cl.putAllOp); assertFalse(cl.isOriginRemote); @@ -1094,7 +1089,6 @@ public void testClientCommitAndDataStoreGetsEvent() throws Exception { } }); - Thread.sleep(10000); client.invoke(new SerializableCallable() { public Object call() throws Exception { Region<CustId, Customer> custRegion = getCache().getRegion(CUSTOMER); @@ -1150,9 +1144,6 @@ public void testClientCommitAndDataStoreGetsEvent() throws Exception { } }); - Thread.sleep(10000); - - /* * Validate nothing came through */ @@ -1199,7 +1190,6 @@ public void testClientCommitAndDataStoreGetsEvent() throws Exception { client.invoke(doAPutInTx); client.invoke(doAnInvalidateInTx); - Thread.sleep(10000); client.invoke(new SerializableCallable() { public Object call() throws Exception { @@ -1365,7 +1355,6 @@ public void testClientCommitAndDataStoreGetsEvent() throws Exception { Customer cust = new Customer("name"+i, "address"+i); pr.put(custId, cust); r.put(i, "value"+i); - Thread.sleep(100); } return null; } @@ -1464,7 +1453,6 @@ public void testClientCommitAndDataStoreGetsEvent() throws Exception { getGemfireCache().getLogger().info("SWAP:putting:"+custId); pr.put(custId, cust); r.put(i, "value"+i); - Thread.sleep(100); } return mgr.getTransactionId(); } @@ -2960,9 +2948,21 @@ public void testClientCommitAndDataStoreGetsEvent() throws Exception { Region r = getCache().getRegion(CUSTOMER); assertNull(r.get(new CustId(101))); mgr.begin(); + final TXStateProxy txState = mgr.getTXState(); + assertTrue(txState.isInProgress()); r.put(new CustId(101), new Customer("name101", "address101")); - TransactionId txId = mgr.suspend(); - Thread.sleep(70*1000); + TransactionId txId = mgr.suspend(TimeUnit.MILLISECONDS); + WaitCriterion waitForTxTimeout = new WaitCriterion() { + public boolean done() { + return !txState.isInProgress(); + } + public String description() { + return "txState stayed in progress indicating that the suspend did not timeout"; + } + }; + // tx should timeout after 1 ms but to deal with loaded machines and thread + // scheduling latency wait for 10 seconds before reporting an error. + DistributedTestCase.waitForCriterion(waitForTxTimeout, 10 * 1000, 10, true); try { mgr.resume(txId); fail("expected exception not thrown"); http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/389e2a8c/gemfire-core/src/test/java/com/gemstone/gemfire/internal/cache/RemoteTransactionDUnitTest.java ---------------------------------------------------------------------- diff --git a/gemfire-core/src/test/java/com/gemstone/gemfire/internal/cache/RemoteTransactionDUnitTest.java b/gemfire-core/src/test/java/com/gemstone/gemfire/internal/cache/RemoteTransactionDUnitTest.java index 0daaafb..a78fab4 100644 --- a/gemfire-core/src/test/java/com/gemstone/gemfire/internal/cache/RemoteTransactionDUnitTest.java +++ b/gemfire-core/src/test/java/com/gemstone/gemfire/internal/cache/RemoteTransactionDUnitTest.java @@ -92,6 +92,7 @@ import dunit.Host; import dunit.SerializableCallable; import dunit.SerializableRunnable; import dunit.VM; +import dunit.DistributedTestCase.WaitCriterion; /** * @author sbawaska @@ -140,7 +141,6 @@ public class RemoteTransactionDUnitTest extends CacheTestCase { @Override public void tearDown2() throws Exception { -// try { Thread.sleep(5000); } catch (InterruptedException e) { } // FOR MANUAL TESTING OF STATS - DON"T KEEP THIS try { invokeInEveryVM(verifyNoTxState); } finally { @@ -3600,15 +3600,21 @@ protected static class ClientListener extends CacheListenerAdapter { } }); - Thread.sleep(10000); client.invoke(new SerializableCallable() { public Object call() throws Exception { Region<CustId, Customer> custRegion = getCache().getRegion(CUSTOMER); Region<OrderId, Order> orderRegion = getCache().getRegion(ORDER); Region<CustId,Customer> refRegion = getCache().getRegion(D_REFERENCE); - ClientListener cl = (ClientListener) custRegion.getAttributes().getCacheListeners()[0]; - getCache().getLogger().info("SWAP:CLIENTinvoked:"+cl.invoked); - assertTrue(cl.invoked); + final ClientListener cl = (ClientListener) custRegion.getAttributes().getCacheListeners()[0]; + WaitCriterion waitForListenerInvocation = new WaitCriterion() { + public boolean done() { + return cl.invoked; + } + public String description() { + return "listener was never invoked"; + } + }; + DistributedTestCase.waitForCriterion(waitForListenerInvocation, 10 * 1000, 10, true); return null; } }); @@ -3715,15 +3721,21 @@ protected static class ClientListener extends CacheListenerAdapter { } }); - Thread.sleep(10000); client.invoke(new SerializableCallable() { public Object call() throws Exception { Region<CustId, Customer> custRegion = getCache().getRegion(CUSTOMER); Region<OrderId, Order> orderRegion = getCache().getRegion(ORDER); Region<CustId,Customer> refRegion = getCache().getRegion(D_REFERENCE); - ClientListener cl = (ClientListener) custRegion.getAttributes().getCacheListeners()[0]; - getCache().getLogger().info("SWAP:CLIENTinvoked:"+cl.invoked); - assertTrue(cl.invoked); + final ClientListener cl = (ClientListener) custRegion.getAttributes().getCacheListeners()[0]; + WaitCriterion waitForListenerInvocation = new WaitCriterion() { + public boolean done() { + return cl.invoked; + } + public String description() { + return "listener was never invoked"; + } + }; + DistributedTestCase.waitForCriterion(waitForListenerInvocation, 10 * 1000, 10, true); return null; } });