pivotal-jbarrett commented on code in PR #7630:
URL: https://github.com/apache/geode/pull/7630#discussion_r860147653
##########
geode-core/src/test/java/org/apache/geode/internal/cache/BucketAdvisorTest.java:
##########
@@ -317,4 +331,38 @@ public void
testGetAllHostingMembersReturnsMemberWhenBucketAdvisorHasTwoProfiles
assertThat(bucketAdvisor.adviseInitialized().size()).isEqualTo(1);
}
+
+ @Test
+ void
removePrimaryDeposePrimaryForColocatedChildrenBeforeReleasePrimaryLock() {
+ when(regionAdvisor.getPartitionedRegion()).thenReturn(partitionedRegion);
+ when(regionAdvisor.getBucket(any(Integer.class))).thenReturn(bucket);
+ when(partitionedRegion.getPartitionAttributes()).thenReturn(new
PartitionAttributesImpl());
+
when(bucket.getDistributionManager()).thenReturn(mock(DistributionManager.class));
+ BucketAdvisor bucketAdvisor =
spy(BucketAdvisor.createBucketAdvisor(bucket, regionAdvisor));
+ InternalDistributedMember member = mock(InternalDistributedMember.class);
+ DistributionManager manager = mock(DistributionManager.class);
+ doReturn(true).when(bucketAdvisor).isPrimary();
+ doReturn(manager).when(bucketAdvisor).getDistributionManager();
+ when(manager.getId()).thenReturn(member);
+ bucketAdvisor.setPrimaryMemberForTest(member);
+ bucketAdvisor.setInitialized();
+ doNothing().when(bucketAdvisor).deposePrimaryForColocatedChildren();
+
+ InOrder order = inOrder(bucketAdvisor);
+ bucketAdvisor.removePrimary(member);
+
+ order.verify(bucketAdvisor).deposePrimaryForColocatedChildren();
+ order.verify(bucketAdvisor).releasePrimaryLock();
+ assertThat(bucketAdvisor.basicGetPrimaryMember()).isNull();
+ }
+
+ @BeforeEach
Review Comment:
If you just construct you mocks directly without the `@Mock` annotation,
like the other tests in this file, you don't need this before/after stuff.
Alternatively you can follow these
[instructions](https://www.baeldung.com/mockito-junit-5-extension) to add
support to JUnit5.
##########
geode-core/src/test/java/org/apache/geode/internal/cache/BucketAdvisorTest.java:
##########
@@ -317,4 +331,38 @@ public void
testGetAllHostingMembersReturnsMemberWhenBucketAdvisorHasTwoProfiles
assertThat(bucketAdvisor.adviseInitialized().size()).isEqualTo(1);
}
+
+ @Test
+ void
removePrimaryDeposePrimaryForColocatedChildrenBeforeReleasePrimaryLock() {
+ when(regionAdvisor.getPartitionedRegion()).thenReturn(partitionedRegion);
+ when(regionAdvisor.getBucket(any(Integer.class))).thenReturn(bucket);
+ when(partitionedRegion.getPartitionAttributes()).thenReturn(new
PartitionAttributesImpl());
+
when(bucket.getDistributionManager()).thenReturn(mock(DistributionManager.class));
+ BucketAdvisor bucketAdvisor =
spy(BucketAdvisor.createBucketAdvisor(bucket, regionAdvisor));
+ InternalDistributedMember member = mock(InternalDistributedMember.class);
+ DistributionManager manager = mock(DistributionManager.class);
+ doReturn(true).when(bucketAdvisor).isPrimary();
+ doReturn(manager).when(bucketAdvisor).getDistributionManager();
+ when(manager.getId()).thenReturn(member);
+ bucketAdvisor.setPrimaryMemberForTest(member);
Review Comment:
If there isn't a way to write this test without punching this hole into the
class can you add a Javadoc to this method why it has to be punched.
##########
geode-core/src/test/java/org/apache/geode/internal/cache/BucketAdvisorTest.java:
##########
@@ -43,10 +48,18 @@
import org.apache.geode.internal.cache.partitioned.Bucket;
import org.apache.geode.internal.cache.partitioned.RegionAdvisor;
-public class BucketAdvisorTest {
+class BucketAdvisorTest {
+ @Mock
+ private PartitionedRegion partitionedRegion;
+ @Mock
+ private Bucket bucket;
+ @Mock
+ private RegionAdvisor regionAdvisor;
Review Comment:
Why allocate these here but not use them in any of the other tests?
##########
geode-core/src/main/java/org/apache/geode/internal/cache/BucketAdvisor.java:
##########
@@ -181,6 +182,14 @@ private BucketAdvisor(Bucket bucket, RegionAdvisor
regionAdvisor) {
redundancyTracker =
new BucketRedundancyTracker(pRegion.getRedundantCopies(),
pRegion.getRedundancyTracker());
resetParentAdvisor(bucket.getId());
+
+ if (parentAdvisor == null) {
Review Comment:
`parentAdvisor` is not `final`, can it be mutated after construction? If so
what impact does that have on the inherited locks, which are `final`? If not,
can it be made `final`?
--
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]
