You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/09/14 06:47:28 UTC
[GitHub] [geode] WeijieEST opened a new pull request, #7857: GEODE-10410: Fix bucket lost during rebalance
WeijieEST opened a new pull request, #7857:
URL: https://github.com/apache/geode/pull/7857
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-->
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
<!-- Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
-->
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] WeijieEST commented on a diff in pull request #7857: GEODE-10410: Fix bucket lost during rebalance
Posted by GitBox <gi...@apache.org>.
WeijieEST commented on code in PR #7857:
URL: https://github.com/apache/geode/pull/7857#discussion_r972577636
##########
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 testColocationBothEnforceLocalMaxMemory() throws UnknownHostException {
Review Comment:
The test case was changed for several reasons.
Test cases are renamed to make them a group(see below description about the test group).
Add MB to data size so the log will print the model with correct data size.
Some comments are chanegd, most to help understand the test case better, and the last comment was wrong.
Description about the test group
The first case test that both of the 2 regions are eviction disabled(this was tested before),
the second and third test cases test one region is eviction enabled,
the second test case tests the none eviction region memory full(this was not tested before),
the third test case tests the eviction region memory full(this was tested before but it should be failed if the test condition was set properly).
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] WeijieEST commented on a diff in pull request #7857: GEODE-10410: Fix bucket lost during rebalance
Posted by GitBox <gi...@apache.org>.
WeijieEST commented on code in PR #7857:
URL: https://github.com/apache/geode/pull/7857#discussion_r972580688
##########
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 testColocationOneEnforceLocalMaxMemoryFull() throws UnknownHostException {
Review Comment:
This seconde test case is newly added, I will explain why the third test case was changed.
Most reasons is like above, special reason for this test case is to improve the test condition.
First region lmm was set 500 before, this will make the outloop memory judege(the one I removed in this PR) pass and make the test case pass.
So I decrease the lmm of the first region, so make the total lmm not enough to hold all the data, this test case will pass with this PR applied, but will fail with the old code.
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] WeijieEST commented on a diff in pull request #7857: GEODE-10410: Fix bucket lost during rebalance
Posted by GitBox <gi...@apache.org>.
WeijieEST commented on code in PR #7857:
URL: https://github.com/apache/geode/pull/7857#discussion_r972577636
##########
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 testColocationBothEnforceLocalMaxMemory() throws UnknownHostException {
Review Comment:
The test case was changed for several reasons.
Test cases are renamed to make them a group together with the newly added test case(see below description about the test group).
Add MB to data size so the log will print the model with correct data size.
Some comments are chanegd, most to help understand the test case better, and the last comment was wrong.
Description about the test group
The first case test that both of the 2 regions are eviction disabled(this was tested before),
the second and third test cases test one region is eviction enabled,
the second test case tests the none eviction region memory full(this was not tested before),
the third test case tests the eviction region memory full(this was tested before but it should be failed if the test condition was set properly).
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on a diff in pull request #7857: GEODE-10410: Fix bucket lost during rebalance
Posted by GitBox <gi...@apache.org>.
albertogpz commented on code in PR #7857:
URL: https://github.com/apache/geode/pull/7857#discussion_r972056347
##########
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 testColocationOneEnforceLocalMaxMemoryFull() throws UnknownHostException {
Review Comment:
This test case is still passing without the fix in MemberRollup.java. Why was it changed?
##########
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 testColocationBothEnforceLocalMaxMemory() throws UnknownHostException {
Review Comment:
This test case is still passing without the fix in MemberRollup.java. Why was it changed?
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] WeijieEST commented on a diff in pull request #7857: GEODE-10410: Fix bucket lost during rebalance
Posted by GitBox <gi...@apache.org>.
WeijieEST commented on code in PR #7857:
URL: https://github.com/apache/geode/pull/7857#discussion_r972577636
##########
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 testColocationBothEnforceLocalMaxMemory() throws UnknownHostException {
Review Comment:
The test case was changed for several reasons.
- Test cases are renamed to make them a group together with the newly added test case(see below description about the test group).
- Add MB to data size so the log will print the model with correct data size.
- Some comments are chanegd, most to help understand the test case better, and the last comment was wrong.
Description about the test group
The first case test that both of the 2 regions are eviction disabled(this was tested before),
the second and third test cases test one region is eviction enabled,
the second test case tests the none eviction region memory full(this was not tested before),
the third test case tests the eviction region memory full(this was tested before but it should be failed if the test condition was set properly).
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] WeijieEST commented on a diff in pull request #7857: GEODE-10410: Fix bucket lost during rebalance
Posted by GitBox <gi...@apache.org>.
WeijieEST commented on code in PR #7857:
URL: https://github.com/apache/geode/pull/7857#discussion_r972577636
##########
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 testColocationBothEnforceLocalMaxMemory() throws UnknownHostException {
Review Comment:
The test case was changed for several reasons.
- Test cases are renamed to make them a group together with the newly added test case(see below description about the test group).
- Add MB to data size so the log will print the model with correct data size.
- Some comments are chanegd, most to help understand the test case better, and the last comment was wrong.
Description about the test group
The first case test that both of the 2 regions are eviction disabled(this was tested before),
the second and third test cases test one region is eviction enabled,
the second test case tests the none eviction region reaches its local max memory(this was not tested before),
the third test case tests the eviction region reaches its local max memory(this was tested before but it should be failed if the test condition was set properly).
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] WeijieEST commented on a diff in pull request #7857: GEODE-10410: Fix bucket lost during rebalance
Posted by GitBox <gi...@apache.org>.
WeijieEST commented on code in PR #7857:
URL: https://github.com/apache/geode/pull/7857#discussion_r972580688
##########
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 testColocationOneEnforceLocalMaxMemoryFull() throws UnknownHostException {
Review Comment:
This seconde test case is newly added, I will explain why the third test case was changed.
Most reasons is like above, special reason for this test case is to improve the test condition.
First region lmm was set 500 before, this will make the memory judege for all regions totally(the one I removed in this PR) pass and make the test case pass.
So I decrease the lmm of the first region, so make the total lmm not enough to hold all the data, this test case will pass with this PR applied, but will fail with the old code.
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz merged pull request #7857: GEODE-10410: Fix bucket lost during rebalance
Posted by GitBox <gi...@apache.org>.
albertogpz merged PR #7857:
URL: https://github.com/apache/geode/pull/7857
--
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on a diff in pull request #7857: GEODE-10410: Fix bucket lost during rebalance
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org