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
