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/04 20:51:06 UTC

[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7442: GEODE-9704: Ensure that register interest is called before ready for events

pivotal-jbarrett commented on code in PR #7442:
URL: https://github.com/apache/geode/pull/7442#discussion_r842089894


##########
geode-core/src/distributedTest/java/org/apache/geode/security/AuthExpirationDUnitTest.java:
##########
@@ -171,8 +170,7 @@ public void registeredInterest_slowReAuth_policyDefault() throws Exception {
   }
 
   @Test
-  @Ignore("unnecessary test case for re-auth, but it manifests GEODE-9704")
-  public void registeredInterest_slowReAuth_policyKeys_durableClient() throws Exception {
+  public void registeredInterest_slowReAuth_policyNone_durableClient() throws Exception {

Review Comment:
   The test name should explain when is being tested and the expected outcome.
   
   



##########
geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java:
##########
@@ -309,6 +316,12 @@ public void serverRemoved(ServerLocation location) {
   }
 
 
+  @TestOnly

Review Comment:
   This annotation alone has little visibility when writing code. I suggest the method name warn developers away from it. If used for testing it should probably be package-private and move the test into the same package. 



##########
geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java:
##########
@@ -109,6 +111,11 @@
   private ScheduledExecutorService recoveryThread;
   private volatile boolean sentClientReady;
 
+  @VisibleForTesting
+  void clearQueueConnections() {

Review Comment:
   Since it is package private it really isn't visible to much other than testing. This annotation is redundant.



##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableRegistrationDUnitTest.java:
##########
@@ -105,132 +106,117 @@ public DurableRegistrationDUnitTest() {
 
   @Override
   public final void postSetUp() throws Exception {
-    Host host = Host.getHost(0);
-    server1VM = host.getVM(0);
-    server2VM = host.getVM(1);
-    durableClientVM = host.getVM(2);
+    server1VM = VM.getVM(0);
+    server2VM = VM.getVM(1);
+    durableClientVM = VM.getVM(2);
+    hostName = VM.getHostName();
     regionName = DurableRegistrationDUnitTest.class.getName() + "_region";
-    CacheServerTestUtil.disableShufflingOfEndpoints();
+    disableShufflingOfEndpoints();
   }
 
   @Test
   public void testSimpleDurableClient() {
 
     // Step 1: Starting the servers
-    PORT1 = server1VM
-        .invoke(() -> CacheServerTestUtil.createCacheServer(regionName, Boolean.TRUE));
-    PORT2 = server2VM
-        .invoke(() -> CacheServerTestUtil.createCacheServer(regionName, Boolean.TRUE));
+    PORT1 = server1VM.invoke(() -> createCacheServer(regionName, Boolean.TRUE));
+    PORT2 = server2VM.invoke(() -> createCacheServer(regionName, Boolean.TRUE));
 
     // Step 2: Bring Up the Client
-    // Start a durable client that is not kept alive on the server when it
-    // stops normally
+    // Start a durable client that is not kept alive on the server when it stops normally
     final String durableClientId = getName() + "_client";
 
     final int durableClientTimeout = 600; // keep the client alive for 600
     // seconds
-    durableClientVM.invoke(() -> CacheServerTestUtil.createCacheClient(
-        getClientPool(NetworkUtils.getServerHostName(durableClientVM.getHost()), PORT1, PORT2, true,
+    durableClientVM.invoke(() -> createCacheClient(
+        getClientPool(hostName, PORT1, PORT2, true,
             0),
         regionName, getClientDistributedSystemProperties(durableClientId, durableClientTimeout),
         Boolean.TRUE));
 
-    // Send clientReady message
-    durableClientVM.invoke(new CacheSerializableRunnable("Send clientReady") {
-      @Override
-      public void run2() throws CacheException {
-        CacheServerTestUtil.getCache().readyForEvents();
-      }
-    });
+
 
     // Step 3: Client registers Interests
     // KEY_STONE1, KEY_STONE2 are registered as durableKeys & KEY_STONE3,
     // KEY_STONE4 as non-durableKeys
+    durableClientVM.invoke(() -> registerKey(K1, Boolean.FALSE));
+    durableClientVM.invoke(() -> registerKey(K2, Boolean.FALSE));
+    durableClientVM.invoke(() -> registerKey(K3, Boolean.TRUE));
+    durableClientVM.invoke(() -> registerKey(K4, Boolean.TRUE));
 
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K1, Boolean.FALSE));
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K2, Boolean.FALSE));
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K3, Boolean.TRUE));
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K4, Boolean.TRUE));
+
+    // Send clientReady message
+    durableClientVM.invoke("Send clientReady", new CacheSerializableRunnable() {
+      @Override
+      public void run2() throws CacheException {
+        getCache().readyForEvents();
+      }
+    });
 
     // Step 4: Update Values on the Server for KEY_STONE1, KEY_STONE2,
     // KEY_STONE3, KEY_STONE4
-
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K1, "Value1"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K2, "Value2"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K3, "Value3"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K4, "Value4"));
+    server2VM.invoke(() -> putValue(K1, "Value1"));
+    server2VM.invoke(() -> putValue(K2, "Value2"));
+    server2VM.invoke(() -> putValue(K3, "Value3"));
+    server2VM.invoke(() -> putValue(K4, "Value4"));
 
     Wait.pause(1000);
-    // Step 5: Verify Updates on the Client
 
-    assertEquals("Value1", server2VM.invoke(() -> DurableRegistrationDUnitTest.getValue(K1)));
-    assertEquals("Value1", server1VM.invoke(() -> DurableRegistrationDUnitTest.getValue(K1)));
+    // Step 5: Verify Updates on the Client
+    assertEquals("Value1", server2VM.invoke(() -> getValue(K1)));
+    assertEquals("Value1", server1VM.invoke(() -> getValue(K1)));
 
-    assertEquals("Value1",
-        durableClientVM.invoke(() -> DurableRegistrationDUnitTest.getValue(K1)));
-    assertEquals("Value2",
-        durableClientVM.invoke(() -> DurableRegistrationDUnitTest.getValue(K2)));
-    assertEquals("Value3",
-        durableClientVM.invoke(() -> DurableRegistrationDUnitTest.getValue(K3)));
-    assertEquals("Value4",
-        durableClientVM.invoke(() -> DurableRegistrationDUnitTest.getValue(K4)));
+    assertEquals("Value1", durableClientVM.invoke(() -> getValue(K1)));
+    assertEquals("Value2", durableClientVM.invoke(() -> getValue(K2)));
+    assertEquals("Value3", durableClientVM.invoke(() -> getValue(K3)));
+    assertEquals("Value4", durableClientVM.invoke(() -> getValue(K4)));
 
     // Step 6: Close Cache of the DurableClient
-    durableClientVM.invoke(DurableRegistrationDUnitTest::closeCache);
-    // pause(5000);
+    durableClientVM.invoke(this::closeCache);
+
     // Step 7: Update KEY_STONE1,KEY_STONE2,KEY_STONE3,KEY_STONE4 on the
     // Server say with values PingPong1, PingPong2, PingPong3, PingPong4
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K1, "PingPong1"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K2, "PingPong2"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K3, "PingPong3"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K4, "PingPong4"));
+    server2VM.invoke(() -> putValue(K1, "PingPong1"));
+    server2VM.invoke(() -> putValue(K2, "PingPong2"));
+    server2VM.invoke(() -> putValue(K3, "PingPong3"));
+    server2VM.invoke(() -> putValue(K4, "PingPong4"));
 
     // Step 8: Re-start the Client
-    durableClientVM
-        .invoke(() -> CacheServerTestUtil.createCacheClient(
-            getClientPool(NetworkUtils.getServerHostName(durableClientVM.getHost()), PORT1, PORT2,
-                true, 0),
-            regionName, getClientDistributedSystemProperties(durableClientId), Boolean.TRUE));
+    durableClientVM.invoke(() -> createCacheClient(
+        getClientPool(hostName, PORT1, PORT2,
+            true, 0),
+        regionName, getClientDistributedSystemProperties(durableClientId), Boolean.TRUE));
 
     // Send clientReady message
-    durableClientVM.invoke(new CacheSerializableRunnable("Send clientReady") {
+    durableClientVM.invoke("Send clientReady", new CacheSerializableRunnable() {

Review Comment:
   Lambda?



##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableRegistrationDUnitTest.java:
##########
@@ -409,59 +369,50 @@ public void run2() throws CacheException {
   public void testDurableClientWithRegistrationHA() {
 
     // Step 1: Start server1
-    PORT1 = server1VM
-        .invoke(() -> CacheServerTestUtil.createCacheServer(regionName, Boolean.TRUE));
+    PORT1 = server1VM.invoke(() -> createCacheServer(regionName, Boolean.TRUE));
     PORT2 = getRandomAvailableTCPPort();
 
     // Step 2: Bring Up the Client
     final String durableClientId = getName() + "_client";
     // keep the client alive for 600 seconds
     final int durableClientTimeout = 600;
-    durableClientVM.invoke(() -> CacheServerTestUtil.createCacheClient(
-        getClientPool(NetworkUtils.getServerHostName(durableClientVM.getHost()), PORT1, PORT2, true,
+    durableClientVM.invoke(() -> createCacheClient(
+        getClientPool(hostName, PORT1, PORT2, true,
             1),
         regionName, getClientDistributedSystemProperties(durableClientId, durableClientTimeout)));
 
     // Send clientReady message
-    durableClientVM.invoke(new CacheSerializableRunnable("Send clientReady") {
+    durableClientVM.invoke("Send clientReady", new CacheSerializableRunnable() {
       @Override
       public void run2() throws CacheException {
-        CacheServerTestUtil.getCache().readyForEvents();
+        getCache().readyForEvents();
       }
     });
 
     // Step 3: Client registers Interests
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K1, Boolean.FALSE));
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K2, Boolean.FALSE));
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K3, Boolean.TRUE));
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K4, Boolean.TRUE));
+    durableClientVM.invoke(() -> registerKey(K1, Boolean.FALSE));
+    durableClientVM.invoke(() -> registerKey(K2, Boolean.FALSE));
+    durableClientVM.invoke(() -> registerKey(K3, Boolean.TRUE));
+    durableClientVM.invoke(() -> registerKey(K4, Boolean.TRUE));
 
     // Step 4: Bring up the server2
-
-    server2VM
-        .invoke(() -> CacheServerTestUtil.createCacheServer(regionName, Boolean.TRUE, PORT2));
+    server2VM.invoke(() -> createCacheServer(regionName, Boolean.TRUE, PORT2));
 
     Wait.pause(3000);
 
     // Check server2 got all the interests registered by the durable client.
-    server2VM.invoke(new CacheSerializableRunnable("Verify Interests.") {
+    server2VM.invoke("Verify Interests.", new CacheSerializableRunnable() {
       @Override
       public void run2() throws CacheException {
-        LogWriterUtils.getLogWriter()
-            .info("### Verifying interests registered by DurableClient. ###");
+        logger.info("### Verifying interests registered by DurableClient. ###");
         CacheClientNotifier ccn = CacheClientNotifier.getInstance();
         CacheClientProxy p = null;
 
         // Get proxy for the client.
         for (int i = 0; i < 60; i++) {
-          Iterator ps = ccn.getClientProxies().iterator();
+          Iterator<CacheClientProxy> ps = ccn.getClientProxies().iterator();

Review Comment:
   👏 Thanks for cleaning up the generics!



##########
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/DurableRegistrationDUnitTest.java:
##########
@@ -247,151 +233,125 @@ public void run2() throws CacheException {
   @Test
   public void testSimpleDurableClientWithRegistration() {
     // Step 1: Starting the servers
-    PORT1 = server1VM
-        .invoke(() -> CacheServerTestUtil.createCacheServer(regionName, Boolean.TRUE));
-    PORT2 = server2VM
-        .invoke(() -> CacheServerTestUtil.createCacheServer(regionName, Boolean.TRUE));
+    PORT1 = server1VM.invoke(() -> createCacheServer(regionName, Boolean.TRUE));
+    PORT2 = server2VM.invoke(() -> createCacheServer(regionName, Boolean.TRUE));
 
     // Step 2: Bring Up the Client
-    // Start a durable client that is not kept alive on the server when it
-    // stops normally
+    // Start a durable client that is not kept alive on the server when it stops normally
     final String durableClientId = getName() + "_client";
     // keep the client alive for 600 seconds
     final int durableClientTimeout = 600;
-    durableClientVM.invoke(() -> CacheServerTestUtil.createCacheClient(
-        getClientPool(NetworkUtils.getServerHostName(durableClientVM.getHost()), PORT1, PORT2, true,
+    durableClientVM.invoke(() -> createCacheClient(
+        getClientPool(hostName, PORT1, PORT2, true,
             0),
         regionName, getClientDistributedSystemProperties(durableClientId, durableClientTimeout)));
 
+    // Step 3: Client registers Interests
+    // KEY_STONE1, KEY_STONE2 are registered as durableKeys & KEY_STONE3,
+    // KEY_STONE4 as non-durableKeys
+
+    durableClientVM.invoke(() -> registerKey(K1, Boolean.FALSE));
+    durableClientVM.invoke(() -> registerKey(K2, Boolean.FALSE));
+    durableClientVM.invoke(() -> registerKey(K3, Boolean.TRUE));
+    durableClientVM.invoke(() -> registerKey(K4, Boolean.TRUE));
+
     // Send clientReady message
-    durableClientVM.invoke(new CacheSerializableRunnable("Send clientReady") {
+    durableClientVM.invoke("Send clientReady", new CacheSerializableRunnable() {
       @Override
       public void run2() throws CacheException {
-        CacheServerTestUtil.getCache().readyForEvents();
+        getCache().readyForEvents();
       }
     });
 
-    // Step 3: Client registers Interests
-    // KEY_STONE1, KEY_STONE2 are registered as durableKeys & KEY_STONE3,
-    // KEY_STONE4 as non-durableKeys
-
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K1, Boolean.FALSE));
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K2, Boolean.FALSE));
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K3, Boolean.TRUE));
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K4, Boolean.TRUE));
-
     // Step 4: Update Values on the Server for KEY_STONE1, KEY_STONE2,
     // KEY_STONE3, KEY_STONE4
-
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K1, "Value1"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K2, "Value2"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K3, "Value3"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K4, "Value4"));
+    server2VM.invoke(() -> putValue(K1, "Value1"));
+    server2VM.invoke(() -> putValue(K2, "Value2"));
+    server2VM.invoke(() -> putValue(K3, "Value3"));
+    server2VM.invoke(() -> putValue(K4, "Value4"));
 
     Wait.pause(1000);
-    // Step 5: Verify Updates on the Client
 
-    assertEquals("Value1", server2VM.invoke(() -> DurableRegistrationDUnitTest.getValue(K1)));
-    assertEquals("Value1", server1VM.invoke(() -> DurableRegistrationDUnitTest.getValue(K1)));
+    // Step 5: Verify Updates on the Client
+    assertEquals("Value1", server2VM.invoke(() -> getValue(K1)));
+    assertEquals("Value1", server1VM.invoke(() -> getValue(K1)));
 
-    assertEquals("Value1",
-        durableClientVM.invoke(() -> DurableRegistrationDUnitTest.getValue(K1)));
-    assertEquals("Value2",
-        durableClientVM.invoke(() -> DurableRegistrationDUnitTest.getValue(K2)));
-    assertEquals("Value3",
-        durableClientVM.invoke(() -> DurableRegistrationDUnitTest.getValue(K3)));
-    assertEquals("Value4",
-        durableClientVM.invoke(() -> DurableRegistrationDUnitTest.getValue(K4)));
+    assertEquals("Value1", durableClientVM.invoke(() -> getValue(K1)));
+    assertEquals("Value2", durableClientVM.invoke(() -> getValue(K2)));
+    assertEquals("Value3", durableClientVM.invoke(() -> getValue(K3)));
+    assertEquals("Value4", durableClientVM.invoke(() -> getValue(K4)));
 
     // Step 6: Close Cache of the DurableClient
-    durableClientVM.invoke(DurableRegistrationDUnitTest::closeCache);
-    // pause(5000);
+    durableClientVM.invoke(this::closeCache);
+
     // Step 7: Update KEY_STONE1,KEY_STONE2,KEY_STONE3,KEY_STONE4 on the
     // Server say with values PingPong1, PingPong2, PingPong3, PingPong4
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K1, "PingPong1"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K2, "PingPong2"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K3, "PingPong3"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K4, "PingPong4"));
+    server2VM.invoke(() -> putValue(K1, "PingPong1"));
+    server2VM.invoke(() -> putValue(K2, "PingPong2"));
+    server2VM.invoke(() -> putValue(K3, "PingPong3"));
+    server2VM.invoke(() -> putValue(K4, "PingPong4"));
 
     // Step 8: Re-start the Client
-    durableClientVM
-        .invoke(() -> CacheServerTestUtil.createCacheClient(
-            getClientPool(NetworkUtils.getServerHostName(durableClientVM.getHost()), PORT1, PORT2,
-                true, 0),
-            regionName, getClientDistributedSystemProperties(durableClientId), Boolean.TRUE));
-
-    // Step 9: Send clientReady message
-    durableClientVM.invoke(new CacheSerializableRunnable("Send clientReady") {
+    durableClientVM.invoke(() -> createCacheClient(
+        getClientPool(hostName, PORT1, PORT2, true, 0),
+        regionName, getClientDistributedSystemProperties(durableClientId), Boolean.TRUE));
+
+
+    // Step 9: Register all Keys
+    durableClientVM.invoke(() -> registerKey(K3, Boolean.TRUE));
+    durableClientVM.invoke(() -> registerKey(K4, Boolean.TRUE));
+
+    durableClientVM.invoke(() -> registerKey(K1, Boolean.FALSE));
+    durableClientVM.invoke(() -> registerKey(K2, Boolean.FALSE));
+
+
+    // Step 10: Send clientReady message
+    durableClientVM.invoke("Send clientReady", new CacheSerializableRunnable() {
       @Override
       public void run2() throws CacheException {
-        CacheServerTestUtil.getCache().readyForEvents();
+        getCache().readyForEvents();
       }
     });
 
-    // pause(1000);
-
-    // Step 10: Register all Keys
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K3, Boolean.TRUE));
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K4, Boolean.TRUE));
-
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K1, Boolean.FALSE));
-    durableClientVM
-        .invoke(() -> DurableRegistrationDUnitTest.registerKey(K2, Boolean.FALSE));
-
     // Step 11: Unregister Some Keys (Here K1, K3)
-    durableClientVM.invoke(() -> DurableRegistrationDUnitTest.unregisterKey(K1));
-    durableClientVM.invoke(() -> DurableRegistrationDUnitTest.unregisterKey(K3));
+    durableClientVM.invoke(() -> unregisterKey(K1));
+    durableClientVM.invoke(() -> unregisterKey(K3));
 
     Wait.pause(5000);
 
     // Step 12: Modify values on the server for all the Keys
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K1, "PingPong_updated_1"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K2, "PingPong_updated_2"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K3, "PingPong_updated_3"));
-    server2VM.invoke(() -> DurableRegistrationDUnitTest.putValue(K4, "PingPong_updated_4"));
+    server2VM.invoke(() -> putValue(K1, "PingPong_updated_1"));
+    server2VM.invoke(() -> putValue(K2, "PingPong_updated_2"));
+    server2VM.invoke(() -> putValue(K3, "PingPong_updated_3"));
+    server2VM.invoke(() -> putValue(K4, "PingPong_updated_4"));
 
     Wait.pause(5000);
 
     // Step 13: Check the values for the ones not unregistered and the
     // Unregistered Keys' Values should be null
     try {
-      assertEquals("PingPong_updated_2",
-          durableClientVM.invoke(() -> DurableRegistrationDUnitTest.getValue(K2)));
+      assertEquals("PingPong_updated_2", durableClientVM.invoke(() -> getValue(K2)));

Review Comment:
   Cleanup with AssertJ.



##########
geode-core/src/main/java/org/apache/geode/cache/client/internal/ReadyForEventsOp.java:
##########
@@ -40,7 +41,8 @@ private ReadyForEventsOp() {
     // no instances allowed
   }
 
-  private static class ReadyForEventsOpImpl extends AbstractOp {
+  @VisibleForTesting

Review Comment:
   Package private static methods don't need `@VisibleForTesting`.



##########
geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java:
##########
@@ -805,10 +815,13 @@ private QueueConnectionImpl createNewPrimary(Set<ServerLocation> excludedServers
       excludedServers.addAll(servers);
     }
 
+    return primary;
+  }
+
+  public void markQueueAsReadyForEvents(QueueConnectionImpl primary) {

Review Comment:
   Perhaps using `@NotNull` annotations on the API. Make the caller guarantee the `primary` parameter is not `null`.



##########
geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/QueueManagerJUnitTest.java:
##########
@@ -312,6 +378,21 @@ private static void assertPortEquals(int[] expected, Iterable<Connection> actual
     assertThat(actualPorts).isEqualTo(expectedPorts);
   }
 
+  private class RecoveryDummyPool extends DummyPool {

Review Comment:
   Let's avoid the term "dummy" please. `TestPool` or `FakePool` would be acceptable. 



##########
geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java:
##########
@@ -678,21 +694,15 @@ private void recoverRedundancy(Set<ServerLocation> excludedServers, boolean reco
             if (recoverInterest && queueConnections.getPrimary() == null
                 && queueConnections.getBackups().isEmpty()) {
               // we lost our queue at some point. We Need to recover
-              // interest. This server will be made primary after this method
-              // finishes
-              // because whoever killed the primary when this method started
-              // should
+              // interest. This server will be made primary after this method finishes
+              // because whoever killed the primary when this method started should
               // have scheduled a task to recover the primary.
               isFirstNewConnection = true;
               // TODO - Actually, we need a better check than the above. There's

Review Comment:
   If the TODO still relevant? Perhaps the comment is old and needs to just go away or more work needs to be done to finish this code.



##########
geode-core/src/test/java/org/apache/geode/internal/cache/LocalRegionUpdateTest.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache;
+
+import static org.apache.geode.internal.statistics.StatisticsClockFactory.disabledClock;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.function.Function;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+import org.mockito.quality.Strictness;
+
+import org.apache.geode.cache.DataPolicy;
+import org.apache.geode.cache.InterestPolicy;
+import org.apache.geode.cache.InterestResultPolicy;
+import org.apache.geode.cache.RegionAttributes;
+import org.apache.geode.cache.SubscriptionAttributes;
+import org.apache.geode.cache.client.internal.PoolImpl;
+import org.apache.geode.cache.client.internal.RegisterInterestTracker;
+import org.apache.geode.cache.client.internal.ServerRegionProxy;
+import org.apache.geode.distributed.internal.DSClock;
+import org.apache.geode.distributed.internal.InternalDistributedSystem;
+import org.apache.geode.internal.cache.tier.InterestType;
+
+public class LocalRegionUpdateTest {
+  private InternalDataView internalDataView;
+  private LocalRegion region;
+  private RegisterInterestTracker registerInterestTracker;
+
+  @Rule
+  public MockitoRule mockitoRule = MockitoJUnit.rule().strictness(Strictness.STRICT_STUBS);
+
+  @Before

Review Comment:
   Use JUnit 5.



##########
geode-core/src/main/java/org/apache/geode/cache/client/internal/RegisterInterestTracker.java:
##########
@@ -106,7 +108,8 @@ public static int getInterestLookupIndex(boolean isDurable, boolean receiveUpdat
     return result;
   }
 
-  void addSingleInterest(final @NotNull LocalRegion r, final @NotNull Object key,
+  @VisibleForTesting
+  public void addSingleInterest(final @NotNull LocalRegion r, final @NotNull Object key,

Review Comment:
   Can the test needing this method be moved into the same package to retain the package-private?



##########
geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/QueueManagerJUnitTest.java:
##########
@@ -74,9 +82,10 @@
   private EndpointManagerImpl endpoints;
   private DummySource source;
   private DummyFactory factory;
-  private QueueManager manager;
+  private QueueManagerImpl manager;
   private ScheduledExecutorService background;
   private PoolStats stats;
+  private List<Op> opList;
 
   @Before

Review Comment:
   Unless this test is using legacy rules please upgrade to JUnit5.



##########
geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java:
##########
@@ -1110,9 +1126,8 @@ private void recoverSingleList(final @NotNull InterestType interestType,
   }
 
   private void recoverCqs(Connection recoveredConnection, boolean isDurable) {
-    Map cqs = getPool().getRITracker().getCqsMap();
-    for (Object o : cqs.entrySet()) {
-      Map.Entry e = (Map.Entry) o;
+    Map<CqQuery, Boolean> cqs = getPool().getRITracker().getCqsMap();
+    for (Map.Entry<CqQuery, Boolean> e : cqs.entrySet()) {
       ClientCQ cqi = (ClientCQ) e.getKey();

Review Comment:
   If the key is going to be blindly cast to `ClientCQ` the isn't this really a `Map<ClientCQ, Boolean>`. 



-- 
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