pivotal-jbarrett commented on code in PR #7608: URL: https://github.com/apache/geode/pull/7608#discussion_r860236520
########## geode-cq/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientCQDUnitTest.java: ########## @@ -963,120 +956,92 @@ private static void verifyDurableCqs(final List<String> durableCqNames, */ @Test public void testReadyForEventsNotCalledImplicitlyWithCacheXML() throws InterruptedException { - try { - setPeriodicACKObserver(durableClientVM); - final String cqName = "cqTest"; - // Start a server - server1Port = - server1VM.invoke(() -> createCacheServerFromXml( - DurableClientTestBase.class.getResource("durablecq-server-cache.xml"))); - - // Start a durable client that is not kept alive on the server when it - // stops normally - final String durableClientId = getName() + "_client"; - // create client cache from xml - durableClientVM.invoke(() -> createCacheClientFromXml( - DurableClientTestBase.class.getResource("durablecq-client-cache.xml"), "client", - durableClientId, VERY_LONG_DURABLE_TIMEOUT_SECONDS, false)); + final String cqName = "cqTest"; + // Start a server + server1Port = + server1VM.invoke(() -> createCacheServerFromXml( + DurableClientTestBase.class.getResource("durablecq-server-cache.xml"))); - // verify that readyForEvents has not yet been called on all the client's pools - durableClientVM.invoke("check readyForEvents not called", new CacheSerializableRunnable() { - @Override - public void run2() throws CacheException { - for (Pool p : PoolManager.getAll().values()) { - assertFalse(((PoolImpl) p).getReadyForEventsCalled()); - } - } - }); + // Start a durable client that is not kept alive on the server when it + // stops normally + final String durableClientId = getName() + "_client"; - // Send clientReady message - sendClientReady(durableClientVM); - registerDurableCq(cqName); + // create client cache from xml + durableClientVM.invoke(() -> createCacheClientFromXml( + DurableClientTestBase.class.getResource("durablecq-client-cache.xml"), "client", + durableClientId, VERY_LONG_DURABLE_TIMEOUT_SECONDS, false)); - // Verify durable client on server1 - verifyDurableClientPresent(VERY_LONG_DURABLE_TIMEOUT_SECONDS, durableClientId, server1VM); + // verify that readyForEvents has not yet been called on all the client's pools + durableClientVM.invoke("check readyForEvents not called", new CacheSerializableRunnable() { + @Override + public void run2() throws CacheException { + for (Pool p : PoolManager.getAll().values()) { + assertThat(((PoolImpl) p).getReadyForEventsCalled()).isFalse(); + } + } + }); - // Start normal publisher client - publisherClientVM.invoke(() -> createCacheClient( - getClientPool(NetworkUtils.getServerHostName(publisherClientVM.getHost()), server1Port, - false), - regionName)); + // Send clientReady message + sendClientReady(durableClientVM); + registerDurableCq(cqName); - // Publish some entries - final int numberOfEntries = 10; - publishEntries(publisherClientVM, 0, numberOfEntries); + // Verify durable client on server1 + verifyDurableClientPresent(VERY_LONG_DURABLE_TIMEOUT_SECONDS, durableClientId, server1VM); - // Verify the durable client received the updates - checkCqListenerEvents(durableClientVM, cqName, numberOfEntries, - VERY_LONG_DURABLE_TIMEOUT_SECONDS); + // Start normal publisher client + publisherClientVM.invoke(() -> createCacheClient( + getClientPool(publisherClientVM.getHost().getHostName(), server1Port, + false), + regionName)); - Thread.sleep(10000); + // Publish some entries + final int numberOfEntries = 10; + publishEntries(publisherClientVM, 0, numberOfEntries); - // Stop the durable client - durableClientVM.invoke(() -> CacheServerTestUtil.closeCache(true)); + // Verify the durable client received the updates + checkCqListenerEvents(durableClientVM, cqName, numberOfEntries, + VERY_LONG_DURABLE_TIMEOUT_SECONDS); - // Verify the durable client still exists on the server - verifyDurableClientPresent(VERY_LONG_DURABLE_TIMEOUT_SECONDS, durableClientId, - server1VM); + Thread.sleep(10000); Review Comment: Still no keen on sleeps and would like to see this await on something. ########## geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientStatsDUnitTest.java: ########## @@ -272,9 +273,12 @@ public static void checkStatisticsWithExpectedValues(int reconnectionCount, int logger.info("Stats:" + "\nDurableReconnectionCount:" + stats.get_durableReconnectionCount() + "\nQueueDroppedCount" + stats.get_queueDroppedCount() + "\nEventsEnqueuedWhileClientAwayCount" + stats.get_eventEnqueuedWhileClientAwayCount()); - assertThat(stats.get_durableReconnectionCount()).isEqualTo(reconnectionCount); - assertThat(stats.get_queueDroppedCount()).isEqualTo(queueDropCount); - assertThat(stats.get_eventEnqueuedWhileClientAwayCount()).isEqualTo(enqueueCount); + await().untilAsserted( + () -> assertThat(stats.get_durableReconnectionCount()).isEqualTo(reconnectionCount)); + await() + .untilAsserted(() -> assertThat(stats.get_queueDroppedCount()).isEqualTo(queueDropCount)); + await().untilAsserted( + () -> assertThat(stats.get_eventEnqueuedWhileClientAwayCount()).isEqualTo(enqueueCount)); } catch (Exception e) { fail("Exception thrown while executing checkStatisticsWithExpectedValues()", e); Review Comment: I am a little hesitant on this `fail`. I assume it was here to bubble up some proper task failure handling but the use of `fail` still smells funny. First off this `catch` block is going to catch that assertion failures and wrap them, which makes me curious about behavior on failure. We don't consistently catch exceptions and convert to failure, so maybe it isn't necessary. If it is I wonder if using the `assertThatCode(...).doesNotThrowAnyException()` is more appropriate replacement? -- 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-unsubscr...@geode.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org