Bugfix/GEODE-10228 DurableClientTestCase.testDurableHAFailover is failing #7608

mhansonp · 2022-04-19T23:19:50Z

There were a couple of basic timing issues with the code.

Starting the cache server is not a synchronous event.
We were not waiting for the queues to clear before restarting, this allowed messages to "persist"
across a restart of the durable client.

Basically all of the problems here are timing issues in the tests.

See my PR comments for key changes

- The test was failing because it didn't wait for the HARegionQueue to clear before shutting down the durable client for test. Thus when it came back up, there was an extra message in the queue. - Reverse the order of readyforevents and registerinterest

adding a close for the control listener

Starting the server is not synchronous.

mhansonp · 2022-04-19T23:21:47Z

...distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientTestCase.java

@@ -429,6 +431,10 @@ public void run2() throws CacheException {
  @Test
  public void testDurableNonHAFailover() {


There were two tests here previously testing two things each.
Broke them out into four tests doing one thing.

mhansonp · 2022-04-19T23:22:23Z

...distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientTestCase.java

      }
    });

-    registerInterest(durableClientVM, regionName, true, InterestResultPolicy.NONE);
+    // Verify the durable client is connected to server1
+    verifyDurableClientPresent(VERY_LONG_DURABLE_TIMEOUT_SECONDS, durableClientId, server1VM);


Verify the servers are up and ready before proceeding.

mhansonp · 2022-04-19T23:23:39Z

...distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientTestCase.java

-    verifyDurableClientPresent(VERY_LONG_DURABLE_TIMEOUT_SECONDS, durableClientId, server1VM);
+    // Wait to until the HARegionQueue has emptied before disconnecting the Durable client
+    // to avoid having left over messages that processed, in particular, the key 0 message
+    waitUntilHARegionQueueSizeIsZero(server1VM);


Wait for the HA region queues to clear before disconnecting the durable client we want to process all the messages first, then disconnect, otherwise we will see them again when the client restarts.

mhansonp · 2022-04-19T23:24:24Z

...distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientTestCase.java


    // Verify the durable client received the updates before failover
    if (redundancyLevel == 1) {
-      checkListenerEvents(4, 1, -1, durableClientVM);
+      checkListenerEvents(3, 1, -1, durableClientVM);


These numbers were contradictory to the required record count because key 0 should not have been there to be counted.

...edTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientBug39997DUnitTest.java

jake-at-work

Thank you for taking a wack at cleanup. I would ask for consistency in the usage of AssertJ over junit assertions. Also, I feel like fail is smell and there is usually an appropriate assertion to replace it. On await please use the untilAsserted and use AssertJ assertions in the block to give meaningful failures.

...stributedTest/java/org/apache/geode/cache/client/ClientServerRegisterInterestsDUnitTest.java

...edTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientBug39997DUnitTest.java

...distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientTestBase.java

kirklund

Agree with all of Jake's comments. Fix those and add in my approval.

...edTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientBug39997DUnitTest.java

Would need to do a lot more changes for this to work which is just not a priority at this moment.

jake-at-work · 2022-04-26T16:55:38Z

@mhansonp As you noticed, you can't convert DUnit based tests to JUni 5. You can rename them to the current naming convention though.

...a/org/apache/geode/internal/cache/tier/sockets/DurableClientNoServerAvailabileDUnitTest.java

...butedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientStatsDUnitTest.java

...tributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientCQDUnitTest.java

...utedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientSimpleDUnitTest.java

jake-at-work · 2022-04-27T17:04:54Z

Can you also please rename the updated tests to their current naming conventions.

jake-at-work

Two minor things to consider but nothing blocking from me now. Thanks!

jake-at-work · 2022-04-27T20:53:20Z

...butedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientStatsDUnitTest.java

+      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);


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?

AssertionError which is thrown by the assertions in this try block is an Error rather than an Exception so it won't be caught here and any assertions that fail will bubble up. I personally dislike the use of try/catch coupled with fail() since it's not really adding anything here that wouldn't be clear from the stack trace of the exception, and it introduces inconsistent behaviour depending on the type of error/exception we might encounter.

jake-at-work · 2022-04-27T20:54:07Z

...tributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientCQDUnitTest.java

-      // Verify the durable client still exists on the server
-      verifyDurableClientPresent(VERY_LONG_DURABLE_TIMEOUT_SECONDS, durableClientId,
-          server1VM);
+    Thread.sleep(10000);


Still no keen on sleeps and would like to see this await on something.

DonalEvans

Nice clean-up! There's a lot of work still to do in these old tests, but some is always better than none, so none of my comments should be considered blocking.

DonalEvans · 2022-04-27T22:11:14Z

...stributedTest/java/org/apache/geode/cache/client/ClientServerRegisterInterestsDUnitTest.java

+  protected <V> V put() {
+    return (V) gemfireServerVm.invoke(() -> {
+      Cache cache = CacheFactory.getAnyInstance();
+      cache.getRegion("/Example").put("2", "TWO");
+      return cache.getRegion("/Example").get("2");
    });
  }


Warnings here can be fixed by using:

protected String put() { return gemfireServerVm.invoke(() -> { Cache cache = CacheFactory.getAnyInstance(); cache.getRegion("/Example").put("2", "TWO"); return cache.<String, String>getRegion("/Example").get("2"); }); }

Since we explicitly create the Region as <String, String>, there's no need to use generics here.

DonalEvans · 2022-04-27T22:20:36Z

...stributedTest/java/org/apache/geode/cache/client/ClientServerRegisterInterestsDUnitTest.java

+    final long timeout = (System.currentTimeMillis()
+        + ClientServerRegisterInterestsDUnitTest.WAIT_TIME_MILLISECONDS);

    while (entryEvents.empty() && (System.currentTimeMillis() < timeout)) {
      synchronized (this) {
        try {
-          TimeUnit.MILLISECONDS.timedWait(this, Math.min(500, waitTimeMilliseconds));
+          TimeUnit.MILLISECONDS.timedWait(this, Math.min(500,
+              ClientServerRegisterInterestsDUnitTest.WAIT_TIME_MILLISECONDS));
        } catch (InterruptedException ignore) {
        }


This whole method should probably just be replaced with:

await().untilAsserted(() -> assertThat(entryEvents).isNotEmpty());

DonalEvans · 2022-04-27T22:35:47Z

...apache/geode/internal/cache/tier/sockets/DurableClientNoServerAvailabileDistributedTest.java

+    final int port = AvailablePortHelper.getRandomAvailableTCPPort();
+    vm0.invoke("create cache", () -> {
+      getSystem(getClientProperties());
+      PoolImpl p = (PoolImpl) PoolManager.createFactory().addServer(hostName, port)


This can be the interface Pool rather than the concrete PoolImpl.

DonalEvans · 2022-04-27T22:40:23Z

...apache/geode/internal/cache/tier/sockets/DurableClientNoServerAvailabileDistributedTest.java

+import org.apache.geode.test.junit.categories.ClientSubscriptionTest;
+
+@Category({ClientSubscriptionTest.class})
+public class DurableClientNoServerAvailabileDistributedTest extends JUnit4CacheTestCase {


Typo here, should be "NoServerAvailable"

DonalEvans · 2022-04-27T22:54:31Z

...butedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientStatsDUnitTest.java

-    startAndCloseNonDurableClientCache(durableClientTimeout);
-    startAndCloseNonDurableClientCache(1); //////// -> Reconnection1
+    startAndCloseNonDurableClientCache();
+    startAndCloseNonDurableClientCache(); //////// -> Reconnection1
    Wait.pause(1400); //////// -> Queue Dropped1


These waits should ideally be replaced with await() calls on some reasonable condition. For this one and the one below it the following seems right:

await().untilAsserted(() -> assertThat(durableClientVM.invoke(() -> getClientCache().isClosed())).isTrue());

DonalEvans · 2022-04-27T23:11:52Z

...butedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientStatsDUnitTest.java

+      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);


AssertionError which is thrown by the assertions in this try block is an Error rather than an Exception so it won't be caught here and any assertions that fail will bubble up. I personally dislike the use of try/catch coupled with fail() since it's not really adding anything here that wouldn't be clear from the stack trace of the exception, and it introduces inconsistent behaviour depending on the type of error/exception we might encounter.

DonalEvans · 2022-04-27T23:48:28Z

geode-dunit/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheServerTestUtil.java

+      Enumeration<?> e = javaSystemProperties.propertyNames();

      while (e.hasMoreElements()) {
        String key = (String) e.nextElement();


This block can be simplified to:

if (javaSystemProperties != null && javaSystemProperties.size() > 0) { for (String key : javaSystemProperties.stringPropertyNames()) { System.setProperty(key, javaSystemProperties.getProperty(key)); } }

DonalEvans · 2022-04-27T23:55:38Z

geode-dunit/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheServerTestUtil.java

    if (javaSystemProperties != null && javaSystemProperties.size() > 0) {
-      Enumeration e = javaSystemProperties.propertyNames();
+      Enumeration<?> e = javaSystemProperties.propertyNames();

      while (e.hasMoreElements()) {
        String key = (String) e.nextElement();


This block can be simplified to:

if (javaSystemProperties != null && javaSystemProperties.size() > 0) { for (String key : javaSystemProperties.stringPropertyNames()) { System.clearProperty(key); } }

No point in checking the size, the for range loop won't do anything if it is empty.

DonalEvans · 2022-04-27T23:57:21Z

geode-dunit/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheServerTestUtil.java

    factory.setScope(Scope.LOCAL);
    factory.setPoolName(p.getName());
-    RegionAttributes attrs = factory.create();
+    RegionAttributes<Object, Object> attrs = factory.create();
    cache.createRegion(regionName1, attrs);
    cache.createRegion(regionName2, attrs);
    pool = p;


The addControlListener arguments to createCacheClientFromXmlN() on line 197 and createCacheClientFromXml() on line 219 are not used and can be removed.

DonalEvans · 2022-04-28T00:00:40Z

geode-dunit/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheServerTestUtil.java

+    /**
+     * This method is not implemented to test event count matches the eventsToCheck.size() which is
+     * confusing. It will wait and return if it got something in the eventsToCheck or not.
+     */


This should either use correct javadoc semantics (add a @param tag for each argument and an @return tag for the returned value) or it should be a block comment rather than a javadoc comment.

I don't agree, I would rather have the hover over documentation with some information than none. Though completing the information should not be that hard.

I just have a bee in my bonnet about javadocs after the big clean-up I did lately, I think. I saw so many places where there was stuff like

/** * Constructs the object * / public SomeClass(int param1, int param2) { ...

that provided no value as a javadoc and caused the linter to throw warnings. I think sometimes people just use javadoc comments because they're green in IntelliJ and so stand out more.

Oh for sure, that comment is pointless. I delete those when I find them. I should have been more precise, I would rather have some useful comments missing params and return tags than no comments at all.

DonalEvans · 2022-04-28T00:13:00Z

...utedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientSimpleDUnitTest.java

@@ -576,6 +577,7 @@ public void run2() throws CacheException {

  @Test
  public void testReadyForEventsNotCalledImplicitlyForRegisterInterestWithCacheXML() {
+    int[] ports = getRandomAvailableTCPPorts(2);


This variable is not used.

mhansonp · 2022-04-28T07:56:00Z

So there is one remaining problem in the stress test and that is caused by the fact that the test DurableClientSimpleDUnitTest.testReadyForEventsNotCalledImplicitlyForRegisterInterestWithCacheXML uses a server with a defined port number of 10188. So when the test starts up it references an cache.xml that has the port defined in it. This means that the test cannot be run in parallel with itself as is done through the stress test and be ok. Depending on timing it will have resource contention. This has been in there for quite some time and is not really a problem except in stress test. If we do want to fix it, we will need to find a way to override it by generating it on the fly or modifying the XML on the fly. Generating it on the fly would be the easier path.

jake-at-work · 2022-04-28T14:46:51Z

@mhansonp I think we can reasonably ignore that repeated test failure.

mhansonp · 2022-04-28T17:41:14Z

I filed https://issues.apache.org/jira/browse/GEODE-10265 for the stress test failure.

GEODE-10265

mhansonp added 3 commits April 18, 2022 21:29

GEODE-10228: Fixing a DurableClientTestCase.testDurableHAFailover

9d3ddac

adding a close for the control listener

GEODE-10228: Fixing a DurableClientTestCase.testDurableHAFailover

cdd126e

Starting the server is not synchronous.

mhansonp commented Apr 19, 2022

View reviewed changes

GEODE-10228: Cleanup

96cd36f

mhansonp marked this pull request as ready for review April 20, 2022 16:44

mhansonp requested review from kirklund, nabarunnag, DonalEvans, agingade, Bill, echobravopapa, kamilla1201 and jake-at-work as code owners April 20, 2022 16:44

onichols-pivotal requested review from albertogpz and mkevo April 20, 2022 16:54

nabarunnag reviewed Apr 20, 2022

View reviewed changes

...edTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientBug39997DUnitTest.java Outdated Show resolved Hide resolved

GEODE-10228: Removing unnecessary statement

3149cca

jake-at-work requested changes Apr 21, 2022

View reviewed changes

kirklund approved these changes Apr 21, 2022

View reviewed changes

GEODE-10228: Cleanup per reviewer comments

c72002f

mhansonp requested a review from jake-at-work April 25, 2022 18:29

jake-at-work reviewed Apr 25, 2022

View reviewed changes

...edTest/java/org/apache/geode/internal/cache/tier/sockets/DurableClientBug39997DUnitTest.java Outdated Show resolved Hide resolved

mhansonp added 2 commits April 25, 2022 15:52

GEODE-10228: Cleanup per comments plus one other change

c5b50d8

GEODE-10228: Cleanup per comments + junit 5 supporting changes

4ecdc3b

mhansonp requested review from jake-at-work and kirklund April 25, 2022 23:42

kirklund approved these changes Apr 25, 2022

View reviewed changes

GEODE-10228: Removing junit5 changes

13a5fe7

Would need to do a lot more changes for this to work which is just not a priority at this moment.

mhansonp mentioned this pull request Apr 26, 2022

GEODE-10155: Avoid threads hanging when function execution times-out #7493

Merged

6 tasks

jake-at-work requested changes Apr 27, 2022

View reviewed changes

GEODE-10228: more cleanup

9b46fa4

jake-at-work approved these changes Apr 27, 2022

View reviewed changes

agingade approved these changes Apr 27, 2022

View reviewed changes

GEODE-10228: fixing a port conflict by using random ports

9d8f193

DonalEvans reviewed Apr 28, 2022

View reviewed changes

GEODE-10228: removing diagnostic changes to track down

ab022e8

GEODE-10265

mhansonp merged commit 5f1d358 into apache:develop Apr 28, 2022

mhansonp deleted the bugfix/GEODE-10228 branch April 28, 2022 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/GEODE-10228 DurableClientTestCase.testDurableHAFailover is failing #7608

Bugfix/GEODE-10228 DurableClientTestCase.testDurableHAFailover is failing #7608

mhansonp commented Apr 19, 2022 •

edited

mhansonp Apr 19, 2022

mhansonp Apr 19, 2022

mhansonp Apr 19, 2022

mhansonp Apr 19, 2022 •

edited

jake-at-work left a comment

kirklund left a comment

jake-at-work commented Apr 26, 2022

jake-at-work commented Apr 27, 2022

jake-at-work left a comment

jake-at-work Apr 27, 2022

DonalEvans Apr 27, 2022

jake-at-work Apr 27, 2022

DonalEvans left a comment

DonalEvans Apr 27, 2022

DonalEvans Apr 27, 2022

DonalEvans Apr 27, 2022

DonalEvans Apr 27, 2022

DonalEvans Apr 27, 2022

DonalEvans Apr 27, 2022

DonalEvans Apr 27, 2022

DonalEvans Apr 27, 2022

jake-at-work Apr 28, 2022

DonalEvans Apr 27, 2022

DonalEvans Apr 28, 2022

jake-at-work Apr 28, 2022

DonalEvans Apr 28, 2022

jake-at-work Apr 28, 2022

DonalEvans Apr 28, 2022

mhansonp commented Apr 28, 2022

jake-at-work commented Apr 28, 2022

mhansonp commented Apr 28, 2022

		@@ -429,6 +431,10 @@ public void run2() throws CacheException {
		@Test
		public void testDurableNonHAFailover() {

Bugfix/GEODE-10228 DurableClientTestCase.testDurableHAFailover is failing #7608

Bugfix/GEODE-10228 DurableClientTestCase.testDurableHAFailover is failing #7608

Conversation

mhansonp commented Apr 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhansonp Apr 19, 2022 • edited

Choose a reason for hiding this comment

jake-at-work left a comment

Choose a reason for hiding this comment

kirklund left a comment

Choose a reason for hiding this comment

jake-at-work commented Apr 26, 2022

jake-at-work commented Apr 27, 2022

jake-at-work left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonalEvans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhansonp commented Apr 28, 2022

jake-at-work commented Apr 28, 2022

mhansonp commented Apr 28, 2022

mhansonp commented Apr 19, 2022 •

edited

mhansonp Apr 19, 2022 •

edited