From 67ebd727bef5c613bfe2aaf4258a5472ac433978 Mon Sep 17 00:00:00 2001 From: WeijieEST <108109958+WeijieEST@users.noreply.github.com> Date: Wed, 21 Sep 2022 01:37:57 +0800 Subject: [PATCH] GEODE-10410: Fix bucket lost during rebalance (#7857) * GEODE-10410: Fix bucket lost during rebalance * improve test case name * improve test case comments and test case names --- .../rebalance/model/MemberRollup.java | 31 ++++--- .../PartitionedRegionLoadModelJUnitTest.java | 82 +++++++++++++++---- 2 files changed, 79 insertions(+), 34 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/MemberRollup.java b/geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/MemberRollup.java index be9c4df2ed45..6078cf028d19 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/MemberRollup.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/MemberRollup.java @@ -131,26 +131,23 @@ public boolean removePrimary(Bucket bucket) { @Override public RefusalReason willAcceptBucket(Bucket bucket, Member source, boolean checkIPAddress) { - RefusalReason reason = super.willAcceptBucket(bucket, source, checkIPAddress); - if (reason.willAccept()) { - BucketRollup bucketRollup = (BucketRollup) bucket; - MemberRollup sourceRollup = (MemberRollup) source; - for (Map.Entry entry : getColocatedMembers().entrySet()) { - String region = entry.getKey(); - Member member = entry.getValue(); - Bucket colocatedBucket = bucketRollup.getColocatedBuckets().get(region); - Member colocatedSource = - sourceRollup == null ? null : sourceRollup.getColocatedMembers().get(region); - if (colocatedBucket != null) { - reason = member.willAcceptBucket(colocatedBucket, colocatedSource, checkIPAddress); - if (!reason.willAccept()) { - return reason; - } + RefusalReason reason; + BucketRollup bucketRollup = (BucketRollup) bucket; + MemberRollup sourceRollup = (MemberRollup) source; + for (Map.Entry entry : getColocatedMembers().entrySet()) { + String region = entry.getKey(); + Member member = entry.getValue(); + Bucket colocatedBucket = bucketRollup.getColocatedBuckets().get(region); + Member colocatedSource = + sourceRollup == null ? null : sourceRollup.getColocatedMembers().get(region); + if (colocatedBucket != null) { + reason = member.willAcceptBucket(colocatedBucket, colocatedSource, checkIPAddress); + if (!reason.willAccept()) { + return reason; } } - return RefusalReason.NONE; } - return reason; + return RefusalReason.NONE; } Map getColocatedMembers() { diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/rebalance/PartitionedRegionLoadModelJUnitTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/rebalance/PartitionedRegionLoadModelJUnitTest.java index 26b2b98b8ece..5423b08a2894 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/rebalance/PartitionedRegionLoadModelJUnitTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/rebalance/PartitionedRegionLoadModelJUnitTest.java @@ -66,7 +66,7 @@ public class PartitionedRegionLoadModelJUnitTest { private static final int MAX_MOVES = 5000; private static final boolean DEBUG = true; - + private static final long MB = 1024 * 1024; private MyBucketOperator bucketOperator; private final PartitionedRegion partitionedRegion = mock(PartitionedRegion.class); final ClusterDistributionManager clusterDistributionManager = @@ -443,7 +443,8 @@ public void testIncompleteColocation() throws UnknownHostException { * lmm, it will prevent a bucket move */ @Test - public void testColocationEnforceLocalMaxMemory() throws UnknownHostException { + public void testColocationTwoNonEvictionRegionsEnforceLocalMaxMemory() + throws UnknownHostException { PartitionedRegionLoadModel model = new PartitionedRegionLoadModel(bucketOperator, 1, 4, getAddressComparor(false), Collections.emptySet(), partitionedRegion); @@ -452,25 +453,27 @@ public void testColocationEnforceLocalMaxMemory() throws UnknownHostException { InternalDistributedMember member2 = new InternalDistributedMember(InetAddress.getByName("127.0.0.1"), 2); - // Create some buckets with low redundancy on member 1 + // Create some buckets with low redundancy on member 1 and enough lmm for region a PartitionMemberInfoImpl details1 = - buildDetails(member1, new long[] {1, 1, 1, 1}, new long[] {1, 1, 1, 1}); + buildDetails(member1, 500, 500 * MB, new long[] {1 * MB, 1 * MB, 1 * MB, 1 * MB}, + new long[] {1, 1, 1, 1}); PartitionMemberInfoImpl details2 = - buildDetails(member2, new long[] {0, 0, 0, 0}, new long[] {0, 0, 0, 0}); + buildDetails(member2, 500, 500 * MB, new long[] {0, 0, 0, 0}, new long[] {0, 0, 0, 0}); model.addRegion("a", Arrays.asList(details1, details2), new FakeOfflineDetails(), true); - // Member 2 has a lmm of 2, so it should only accept 2 buckets + // Region b has a lmm of 2MB, so member2 should only accept 2 buckets PartitionMemberInfoImpl bDetails1 = - buildDetails(member1, 2, 2, new long[] {1, 1, 1, 1}, new long[] {1, 1, 1, 1}); + buildDetails(member1, 2, 2 * MB, new long[] {1 * MB, 1 * MB, 1 * MB, 1 * MB}, + new long[] {1, 1, 1, 1}); PartitionMemberInfoImpl bDetails2 = - buildDetails(member2, 2, 2, new long[] {0, 0, 0, 0}, new long[] {0, 0, 0, 0}); + buildDetails(member2, 2, 2 * MB, new long[] {0, 0, 0, 0}, new long[] {0, 0, 0, 0}); model.addRegion("b", Arrays.asList(bDetails1, bDetails2), new FakeOfflineDetails(), true); assertThat(doMoves(new CompositeDirector(true, true, false, true), model)).isEqualTo(4); - // Everything should be create on member2 + // Only (2+2)MB data should be create on member2 Set expectedCreates = new HashSet<>(); expectedCreates.add(new Create(member2, 0)); expectedCreates.add(new Create(member2, 1)); @@ -483,10 +486,12 @@ public void testColocationEnforceLocalMaxMemory() throws UnknownHostException { } /** - * Test that each region individually honors it's enforce local max memory flag. + * Test that a region with enforceLocalMaxMemory disabled colocated with + * a region with memory full and enforceLocalmaxMemory enabled will prevent a bucket move. */ @Test - public void testColocationIgnoreEnforceLocalMaxMemory() throws UnknownHostException { + public void testColocationOneNonEvictionRegionReachesLocalMaxMemoryLimit() + throws UnknownHostException { PartitionedRegionLoadModel model = new PartitionedRegionLoadModel(bucketOperator, 1, 4, getAddressComparor(false), Collections.emptySet(), partitionedRegion); InternalDistributedMember member1 = @@ -494,19 +499,62 @@ public void testColocationIgnoreEnforceLocalMaxMemory() throws UnknownHostExcept InternalDistributedMember member2 = new InternalDistributedMember(InetAddress.getByName("127.0.0.1"), 2); - // Create some buckets with low redundancy on member 1 PartitionMemberInfoImpl details1 = - buildDetails(member1, new long[] {1, 1, 1, 1}, new long[] {1, 1, 1, 1}); + buildDetails(member1, 1, 8 * MB, new long[] {1 * MB, 1 * MB, 1 * MB, 1 * MB}, + new long[] {1, 1, 1, 1}); PartitionMemberInfoImpl details2 = - buildDetails(member2, new long[] {0, 0, 0, 0}, new long[] {0, 0, 0, 0}); + buildDetails(member2, 1, 8 * MB, new long[] {0, 0, 0, 0}, new long[] {0, 0, 0, 0}); + model.addRegion("a", Arrays.asList(details1, details2), new FakeOfflineDetails(), false); + + + PartitionMemberInfoImpl bDetails1 = + buildDetails(member1, 1, 2 * MB, new long[] {1 * MB, 1 * MB, 1 * MB, 1 * MB}, + new long[] {1, 1, 1, 1}); + PartitionMemberInfoImpl bDetails2 = + buildDetails(member2, 1, 2 * MB, new long[] {0, 0, 0, 0}, new long[] {0, 0, 0, 0}); + model.addRegion("b", Arrays.asList(bDetails1, bDetails2), new FakeOfflineDetails(), true); + + + assertThat(doMoves(new CompositeDirector(true, true, false, true), model)).isEqualTo(4); + + Set expectedCreates = new HashSet<>(); + expectedCreates.add(new Create(member2, 0)); + expectedCreates.add(new Create(member2, 1)); + assertThat(new HashSet<>(bucketOperator.creates)).isEqualTo(expectedCreates); + + Set expectedMoves = new HashSet<>(); + expectedMoves.add(new Move(member1, member2)); + expectedMoves.add(new Move(member1, member2)); + assertThat(new HashSet<>(bucketOperator.primaryMoves)).isEqualTo(expectedMoves); + } + + /** + * Test that a region with memory full and enforceLocalMaxMemory disabled will not prevent a + * bucket move. + */ + @Test + public void testColocationOneEvictionRegionReachesLocalMaxMemoryLimit() + throws UnknownHostException { + PartitionedRegionLoadModel model = new PartitionedRegionLoadModel(bucketOperator, 1, 4, + getAddressComparor(false), Collections.emptySet(), partitionedRegion); + InternalDistributedMember member1 = + new InternalDistributedMember(InetAddress.getByName("127.0.0.1"), 1); + InternalDistributedMember member2 = + new InternalDistributedMember(InetAddress.getByName("127.0.0.1"), 2); + + PartitionMemberInfoImpl details1 = + buildDetails(member1, 1, 4 * MB, new long[] {1 * MB, 1 * MB, 1 * MB, 1 * MB}, + new long[] {1, 1, 1, 1}); + PartitionMemberInfoImpl details2 = + buildDetails(member2, 1, 4 * MB, new long[] {0, 0, 0, 0}, new long[] {0, 0, 0, 0}); model.addRegion("a", Arrays.asList(details1, details2), new FakeOfflineDetails(), true); - // Member 2 has a lmm of 2, so it should only accept 2 buckets PartitionMemberInfoImpl bDetails1 = - buildDetails(member1, 2, 2, new long[] {1, 1, 1, 1}, new long[] {1, 1, 1, 1}); + buildDetails(member1, 1, 2 * MB, new long[] {1 * MB, 1 * MB, 1 * MB, 1 * MB}, + new long[] {1, 1, 1, 1}); PartitionMemberInfoImpl bDetails2 = - buildDetails(member2, 2, 2, new long[] {0, 0, 0, 0}, new long[] {0, 0, 0, 0}); + buildDetails(member2, 1, 2 * MB, new long[] {0, 0, 0, 0}, new long[] {0, 0, 0, 0}); model.addRegion("b", Arrays.asList(bDetails1, bDetails2), new FakeOfflineDetails(), false);