albertogpz commented on code in PR #7857: URL: https://github.com/apache/geode/pull/7857#discussion_r973919611
########## geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/rebalance/PartitionedRegionLoadModelJUnitTest.java: ########## @@ -483,30 +485,73 @@ 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 that memory full with enforceLocalMaxMemory enabled will prevent a bucket move. */ @Test - public void testColocationIgnoreEnforceLocalMaxMemory() throws UnknownHostException { + public void testColocationOneNoneEvictionRegionReachLocalMaxMemoryLimit() throws UnknownHostException { Review Comment: I would rename the test case to `testColocationOneNonEvictionRegionReachesLocalMaxMemoryLimit()` ########## geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/rebalance/PartitionedRegionLoadModelJUnitTest.java: ########## @@ -483,30 +485,73 @@ 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 that memory full with enforceLocalMaxMemory enabled will prevent a bucket move. */ @Test - public void testColocationIgnoreEnforceLocalMaxMemory() throws UnknownHostException { + public void testColocationOneNoneEvictionRegionReachLocalMaxMemoryLimit() 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); - // 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<Create> expectedCreates = new HashSet<>(); + expectedCreates.add(new Create(member2, 0)); + expectedCreates.add(new Create(member2, 1)); + assertThat(new HashSet<>(bucketOperator.creates)).isEqualTo(expectedCreates); + + Set<Move> 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 that memory full with enforceLocalMaxMemory disabled will not prevent a + * bucket move. + */ + @Test + public void testColocationOneEvictionRegionReachLocalMaxMemoryLimit() throws UnknownHostException { Review Comment: I would rename the test case to: `testColocationOneEvictionRegionReachesLocalMaxMemoryLimit()` ########## geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/rebalance/PartitionedRegionLoadModelJUnitTest.java: ########## @@ -483,30 +485,73 @@ 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 that memory full with enforceLocalMaxMemory enabled will prevent a bucket move. */ @Test - public void testColocationIgnoreEnforceLocalMaxMemory() throws UnknownHostException { + public void testColocationOneNoneEvictionRegionReachLocalMaxMemoryLimit() 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); - // 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<Create> expectedCreates = new HashSet<>(); + expectedCreates.add(new Create(member2, 0)); + expectedCreates.add(new Create(member2, 1)); + assertThat(new HashSet<>(bucketOperator.creates)).isEqualTo(expectedCreates); + + Set<Move> 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 that memory full with enforceLocalMaxMemory disabled will not prevent a Review Comment: I would change this to: Test that a region with memory full and enforceLocalMaxMemory... ########## geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/rebalance/PartitionedRegionLoadModelJUnitTest.java: ########## @@ -443,7 +443,7 @@ public void testIncompleteColocation() throws UnknownHostException { * lmm, it will prevent a bucket move */ @Test - public void testColocationEnforceLocalMaxMemory() throws UnknownHostException { + public void testColocationTwoNoneEvictionRegions() throws UnknownHostException { Review Comment: I would rename the test case to: `testColocationTwoNonEvictionRegionsEnforceLocalMaxMemory()` ########## geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/rebalance/PartitionedRegionLoadModelJUnitTest.java: ########## @@ -483,30 +485,73 @@ 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 that memory full with enforceLocalMaxMemory enabled will prevent a bucket move. Review Comment: I would change this sentence to: a region with memory full and enforceLocalmaxMemory enabled... -- 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-unsubscr...@geode.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org