DonalEvans commented on a change in pull request #7144:
URL: https://github.com/apache/geode/pull/7144#discussion_r760658369
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
##########
@@ -1981,6 +1982,36 @@ public void run() {
return recipients;
}
+
+ private Set<InternalDistributedMember>
getAllRecipientsForEvent(InternalCache cache,
+ String partitionedRegionName, int bucketId) {
+ Set recipients = new ObjectOpenHashSet();
Review comment:
Compiler warnings here can be fixed by using
`Set<InternalDistributedMember> recipients = new ObjectOpenHashSet<>();`
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderQueue.java
##########
@@ -1206,7 +1206,8 @@ public void
sendQueueRemovalMesssageForDroppedEvent(PartitionedRegion prQ, int b
temp.put(prQ.getFullPath(), bucketIdToDispatchedKeys);
addRemovedEventToMap(bucketIdToDispatchedKeys, bucketId, key);
Set<InternalDistributedMember> recipients =
- removalThread.getAllRecipients(sender.getCache(), temp);
+ removalThread.getAllRecipientsForEvent(sender.getCache(),
prQ.getFullPath(), bucketId);
Review comment:
This method can be optimized by moving everything above this line inside
the if statement. That way, if there are no recipients, then we don't do all
the extra allocations and work for nothing.
##########
File path:
geode-wan/src/distributedTest/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderOperationsDUnitTest.java
##########
@@ -1514,10 +1596,34 @@ private void waitForSendersRunning() {
vm7.invoke(() -> waitForSenderRunningState("ln"));
}
+ private void waitForSenderNonRunning() {
Review comment:
This would be better named "waitForAllSendersNotRunning"
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/BucketAdvisorTest.java
##########
@@ -228,4 +228,53 @@ public void isShadowBucketDestroyedShouldReturnCorrectly()
{
// Unknown Shadow Bucket
assertThat(bucketAdvisor.isShadowBucketDestroyed(SEPARATOR +
"b5")).isFalse();
}
+
+ @Test
+ public void testGetAllHostingMembersReturnsNoMember() throws Exception {
+ DistributionManager distributionManager = mock(DistributionManager.class);
+ when(distributionManager.getId()).thenReturn(new
InternalDistributedMember("localhost", 321));
+
+ Bucket bucket = mock(Bucket.class);
+ when(bucket.isHosting()).thenReturn(true);
+ when(bucket.isPrimary()).thenReturn(false);
+ when(bucket.getDistributionManager()).thenReturn(distributionManager);
+
+ PartitionedRegion partitionedRegion = mock(PartitionedRegion.class);
+ when(partitionedRegion.getRedundantCopies()).thenReturn(0);
+ when(partitionedRegion.getPartitionAttributes()).thenReturn(new
PartitionAttributesImpl());
+ RegionAdvisor regionAdvisor = mock(RegionAdvisor.class);
+ when(regionAdvisor.getPartitionedRegion()).thenReturn(partitionedRegion);
+
+ BucketAdvisor bucketAdvisor = BucketAdvisor.createBucketAdvisor(bucket,
regionAdvisor);
+ assertThat(bucketAdvisor.getAllHostingMembers().isEmpty()).isTrue();
+ }
+
+ @Test
+ public void testGetAllHostingMembersReturnsOneMember() throws Exception {
Review comment:
`Exception` is never thrown from this method, so this can be removed.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/BucketAdvisorTest.java
##########
@@ -228,4 +228,53 @@ public void isShadowBucketDestroyedShouldReturnCorrectly()
{
// Unknown Shadow Bucket
assertThat(bucketAdvisor.isShadowBucketDestroyed(SEPARATOR +
"b5")).isFalse();
}
+
+ @Test
+ public void testGetAllHostingMembersReturnsNoMember() throws Exception {
Review comment:
`Exception` is never thrown from this method, so this can be removed.
Also, this test name could be more descriptive by including the conditions
that lead to this behaviour. As it currently is, it implies that we expect that
`getAllHostingMembers` always returns no members, which is confusing when
considering the other test below, which implies the opposite. Maybe something
like "getAllHostingMembersReturnsNoMembersWhenBucketAdvisorHasNoProfiles" for
this test and
"getAllHostingMembersReturnsMemberWhenBucketAdvisorHasOneProfileWithHostingBucket".
It would also be good to add a test for the case where the bucket advisor
has two profiles; one for a bucket which is hosting and one for a bucket which
is not hosting, to show that only the first is returned.
##########
File path:
geode-wan/src/distributedTest/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderOperationsDUnitTest.java
##########
@@ -1185,6 +1188,85 @@ public void
testParallelGWSenderUpdateFiltersWhilePuttingOnOneDispatcThread() th
}
+ /**
+ * Put entries in region after gateway sander is stopped. Count number of
PQRM messages sent.
+ */
+ @Test
+ public void
testDroppedEventsSignalizationToSecundaryQueueWhileSenderStopped() throws
Exception {
+ int[] locatorPorts = createLNAndNYLocatorsReturnPrimitives();
+ int lnPort = locatorPorts[0];
+ int nyPort = locatorPorts[1];
Review comment:
Since the `createLNAndNYLocatorsReturnPrimitives()` method is only used
here, it's probably simpler to just inline it:
```
int lnPort = vm0.invoke(() -> createFirstLocatorWithDSId(1));
int nyPort = vm1.invoke(() -> createFirstRemoteLocator(2, lnPort));
```
##########
File path:
geode-wan/src/distributedTest/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderOperationsDUnitTest.java
##########
@@ -1185,6 +1188,85 @@ public void
testParallelGWSenderUpdateFiltersWhilePuttingOnOneDispatcThread() th
}
+ /**
+ * Put entries in region after gateway sander is stopped. Count number of
PQRM messages sent.
+ */
+ @Test
+ public void
testDroppedEventsSignalizationToSecundaryQueueWhileSenderStopped() throws
Exception {
Review comment:
`Exception` is never thrown from this method, so this can be removed.
--
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]
