pivotal-jbarrett commented on code in PR #7442:
URL: https://github.com/apache/geode/pull/7442#discussion_r842906986
##########
geode-core/src/main/java/org/apache/geode/cache/client/internal/RegisterInterestTracker.java:
##########
@@ -302,6 +304,33 @@ private RegionInterestEntry readRegionInterests(String
regionName,
return mapOfInterest.get(regionName);
}
+ public boolean hasInterestsWithResultPolicy(final @NotNull String
regionName, boolean isDurable,
+ final @NotNull InterestResultPolicy interestResultPolicy) {
+ // Iterate InterestTypes searching for any with the input
interestResultPolicy
Review Comment:
This seems like the Javadoc for the method.
##########
geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java:
##########
@@ -805,10 +818,13 @@ private QueueConnectionImpl
createNewPrimary(Set<ServerLocation> excludedServers
excludedServers.addAll(servers);
}
+ return primary;
+ }
+
+ public void markQueueAsReadyForEvents(@NotNull QueueConnectionImpl primary) {
if (primary != null && sentClientReady && primary.sendClientReady()) {
Review Comment:
With `primary` tagged as `@NotNull` you should see a static analyzer warning
that this null check is redundant. The callers should be lit up if they have
potential nulls.
##########
geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/QueueManagerJUnitTest.java:
##########
@@ -35,22 +36,27 @@
import java.nio.ByteBuffer;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Properties;
import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.atomic.AtomicInteger;
-import org.junit.After;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.experimental.categories.Category;
+import org.junit.jupiter.api.AfterEach;
Review Comment:
👍
##########
geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/QueueManagerJUnitTest.java:
##########
@@ -294,6 +295,70 @@ public void
testAddToConnectionListCallsCloseConnectionOpWithKeepAliveTrue2() {
assertThat(connection.keepAlive).isTrue();
}
+ @Test
+ public void recoverPrimaryRegistersBeforeSendingReady() {
+ Set<ServerLocation> excludedServers = new HashSet<>();
+ excludedServers.add(new ServerLocation("localhost", 1));
+ excludedServers.add(new ServerLocation("localhost", 2));
+ excludedServers.add(new ServerLocation("localhost", 3));
+ factory.addConnection(0, 0, 1);
+ factory.addConnection(0, 0, 2);
+ factory.addConnection(0, 0, 3);
+
+ LocalRegion testRegion = mock(LocalRegion.class);
+
+ InternalPool pool = new RecoveryTestPool();
+ ServerRegionProxy serverRegionProxy = new ServerRegionProxy("region",
pool);
+
+ when(testRegion.getServerProxy()).thenReturn(serverRegionProxy);
+ RegionAttributes<Object, Object> regionAttributes =
mock(RegionAttributes.class);
+ when(testRegion.getAttributes()).thenReturn(regionAttributes);
+ when(regionAttributes.getDataPolicy()).thenReturn(DataPolicy.DEFAULT);
+
+ createRegisterInterestTracker(pool, testRegion);
+
+ manager = new QueueManagerImpl(pool, endpoints, source, factory, 2,
+ 20, logger, ClientProxyMembershipID.getNewProxyMembership(ds));
+ manager.start(background);
+ manager.setSendClientReadyInTestOnly();
+ manager.clearQueueConnections();
+ factory.addConnection(0, 0, 4);
+ manager.recoverPrimary(excludedServers);
+
+
assertThat(opList.get(0)).isInstanceOf(RegisterInterestListOp.RegisterInterestListOpImpl.class);
Review Comment:
New tests should be using AssertJ. If the conversion for the rest of the
class is easy please do that as well.
##########
geode-core/src/integrationTest/java/org/apache/geode/cache/client/internal/QueueManagerJUnitTest.java:
##########
@@ -59,26 +65,28 @@
import org.apache.geode.distributed.DistributedSystem;
import org.apache.geode.distributed.internal.ServerLocation;
import
org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.LocalRegion;
import org.apache.geode.internal.cache.PoolStats;
+import org.apache.geode.internal.cache.tier.InterestType;
import org.apache.geode.internal.cache.tier.sockets.ClientProxyMembershipID;
import org.apache.geode.internal.cache.tier.sockets.ServerQueueStatus;
import org.apache.geode.internal.logging.LocalLogWriter;
-import org.apache.geode.test.junit.categories.ClientServerTest;
-@Category(ClientServerTest.class)
+@Tag("ClientServerTest")
public class QueueManagerJUnitTest {
Review Comment:
Please update the test name to `QueueManagerIntegrationTest`.
##########
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:
Even more reason to just make it disappear. Clearly it is "functional"
without whatever someone thought needed to be there.
##########
geode-core/src/main/java/org/apache/geode/cache/client/internal/RegisterInterestTracker.java:
##########
@@ -302,6 +304,33 @@ private RegionInterestEntry readRegionInterests(String
regionName,
return mapOfInterest.get(regionName);
}
+ public boolean hasInterestsWithResultPolicy(final @NotNull String
regionName, boolean isDurable,
+ final @NotNull InterestResultPolicy interestResultPolicy) {
+ // Iterate InterestTypes searching for any with the input
interestResultPolicy
+ return Stream.of(InterestType.values())
+ .anyMatch(interestType -> hasInterestsWithResultPolicy(regionName,
isDurable,
+ interestResultPolicy, interestType));
+ }
+
+ public boolean hasInterestsWithResultPolicy(final @NotNull String
regionName, boolean isDurable,
+ final @NotNull InterestResultPolicy interestResultPolicy,
+ final @NotNull InterestType interestType) {
+ // Check the RegionInterestEntries with receiveUpdatesAsInvalidates both
true and false
Review Comment:
This seems like the Javadoc for the method.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
