You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/04/25 17:10:52 UTC

[GitHub] [geode] kirklund commented on a diff in pull request #7515: GEODE-10020: For Ping task avoid registering new destination endpoint

kirklund commented on code in PR #7515:
URL: https://github.com/apache/geode/pull/7515#discussion_r857846935


##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/cache/wan/SeveralGatewayReceiversWithSamePortAndHostnameForSendersTest.java:
##########
@@ -214,6 +215,56 @@ public void testTwoSendersWithSameIdShouldUseSameValueForEnforceThreadsConnectTo
 
   }
 
+
+  /**
+   * The aim of this test is verify that when several gateway receivers in a remote site share the
+   * same port and hostname-for-senders, the pings sent from the gateway senders reach the right
+   * gateway receiver and not just any of the receivers. Check that only one destination will be
+   * pinged.
+   */
+  @Test
+  public void testPingsToReceiversWithSamePortAndHostnameForSendersReachTheRightReceiver()
+      throws InterruptedException {
+    String senderId = "ln";
+    String regionName = "region-wan";
+    final int remoteLocPort = docker.getExternalPortForService("haproxy", 20334);
+
+    int locPort = createLocator(VM.getVM(0), 1, remoteLocPort);
+
+    VM vm1 = VM.getVM(1);

Review Comment:
   I notice that this is a dunit test in the `acceptanceTest` src set. It should probably be in `distributedTest` instead. The acceptance tests are all written using GfshRule without dunit.
   
   Test names should reflect the test types:
   
   Unit test: MyTest
   
   - Integration test: `MyIntegrationTest`
   - Distributed test: `MyDistributedTest`
   - Acceptance test: `MyAcceptanceTest`



##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/cache/wan/SeveralGatewayReceiversWithSamePortAndHostnameForSendersTest.java:
##########
@@ -214,6 +215,56 @@ public void testTwoSendersWithSameIdShouldUseSameValueForEnforceThreadsConnectTo
 
   }
 
+
+  /**
+   * The aim of this test is verify that when several gateway receivers in a remote site share the
+   * same port and hostname-for-senders, the pings sent from the gateway senders reach the right
+   * gateway receiver and not just any of the receivers. Check that only one destination will be
+   * pinged.
+   */
+  @Test
+  public void testPingsToReceiversWithSamePortAndHostnameForSendersReachTheRightReceiver()
+      throws InterruptedException {
+    String senderId = "ln";
+    String regionName = "region-wan";
+    final int remoteLocPort = docker.getExternalPortForService("haproxy", 20334);
+
+    int locPort = createLocator(VM.getVM(0), 1, remoteLocPort);
+
+    VM vm1 = VM.getVM(1);
+    createCache(vm1, locPort);
+
+    // We use one dispatcher thread. With just one dispatcher thread, only one
+    // connection will be created by the sender towards one of the receivers and it will be
+    // monitored by the one ping thread for that remote receiver.
+    createGatewaySender(vm1, senderId, 2, true, 5,
+        1, GatewaySender.DEFAULT_ORDER_POLICY);
+
+    createPartitionedRegion(vm1, regionName, senderId, 0, 10);
+
+    int NUM_PUTS = 1;
+
+    putKeyValues(vm1, NUM_PUTS, regionName);
+
+    await().untilAsserted(() -> assertThat(getQueuedEvents(vm1, senderId)).isEqualTo(0));
+
+
+    // Wait longer than the value set in the receivers for
+    // maximum-time-between-pings: 10000 (see geode-starter-create.gfsh)
+    // to verify that connections are not closed
+    // by the receivers because each has received the pings timely.
+    int maxTimeBetweenPingsInReceiver = 15000;
+    Thread.sleep(maxTimeBetweenPingsInReceiver);
+
+    int senderPoolDisconnects = getSenderPoolDisconnects(vm1, senderId);
+    assertEquals(0, senderPoolDisconnects);
+
+    int poolEndPointSize = getPoolEndPointSize(vm1, senderId);
+    assertEquals(1, poolEndPointSize);

Review Comment:
   Please always use AssertJ assertions instead of JUnit assertions.



##########
geode-assembly/src/acceptanceTest/java/org/apache/geode/cache/wan/SeveralGatewayReceiversWithSamePortAndHostnameForSendersTest.java:
##########
@@ -214,6 +215,56 @@ public void testTwoSendersWithSameIdShouldUseSameValueForEnforceThreadsConnectTo
 
   }
 
+
+  /**
+   * The aim of this test is verify that when several gateway receivers in a remote site share the
+   * same port and hostname-for-senders, the pings sent from the gateway senders reach the right
+   * gateway receiver and not just any of the receivers. Check that only one destination will be
+   * pinged.
+   */
+  @Test
+  public void testPingsToReceiversWithSamePortAndHostnameForSendersReachTheRightReceiver()
+      throws InterruptedException {
+    String senderId = "ln";
+    String regionName = "region-wan";
+    final int remoteLocPort = docker.getExternalPortForService("haproxy", 20334);
+
+    int locPort = createLocator(VM.getVM(0), 1, remoteLocPort);
+
+    VM vm1 = VM.getVM(1);
+    createCache(vm1, locPort);
+
+    // We use one dispatcher thread. With just one dispatcher thread, only one
+    // connection will be created by the sender towards one of the receivers and it will be
+    // monitored by the one ping thread for that remote receiver.
+    createGatewaySender(vm1, senderId, 2, true, 5,
+        1, GatewaySender.DEFAULT_ORDER_POLICY);
+
+    createPartitionedRegion(vm1, regionName, senderId, 0, 10);
+
+    int NUM_PUTS = 1;
+
+    putKeyValues(vm1, NUM_PUTS, regionName);
+
+    await().untilAsserted(() -> assertThat(getQueuedEvents(vm1, senderId)).isEqualTo(0));
+
+
+    // Wait longer than the value set in the receivers for
+    // maximum-time-between-pings: 10000 (see geode-starter-create.gfsh)
+    // to verify that connections are not closed
+    // by the receivers because each has received the pings timely.
+    int maxTimeBetweenPingsInReceiver = 15000;
+    Thread.sleep(maxTimeBetweenPingsInReceiver);

Review Comment:
   `Thread.sleep` almost always results in flaky tests that fail intermittently. See if you can figure out a different way to test this behavior using `Awaitility` instead.



-- 
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@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org