DonalEvans commented on a change in pull request #7124:
URL: https://github.com/apache/geode/pull/7124#discussion_r781530723



##########
File path: 
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/control/RebalanceOperationComplexDistributedTest.java
##########
@@ -204,9 +201,14 @@ public void testThreeZonesHaveTwoCopiesAfterRebalance() 
throws Exception {
     int zoneABucketCount = getBucketCount(1);
     int zoneBBucketCount = getBucketCount(2);
     int zoneCBucketCount = getBucketCount(3);
-    
assertThat(zoneABucketCount).isGreaterThanOrEqualTo(75).isLessThanOrEqualTo(76);
-    
assertThat(zoneBBucketCount).isGreaterThanOrEqualTo(75).isLessThanOrEqualTo(76);
-    
assertThat(zoneCBucketCount).isGreaterThanOrEqualTo(75).isLessThanOrEqualTo(76);
+    final int LOWER_BOUND = 75;
+    final int UPPER_BOUND = 76;

Review comment:
       These values could be more explicit by being:
   ```
   final int LOWER_BOUND = (EXPECTED_BUCKET_COUNT * 2) / 3
   final int UPPER_BOUND = 1 + (EXPECTED_BUCKET_COUNT * 2) / 3
   ```
   since we have redundancy = 1 (so two instances of each bucket) and three 
servers

##########
File path: 
geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/rebalance/PartitionedRegionLoadModelJUnitTest.java
##########
@@ -1417,9 +1467,16 @@ public void 
testTwoZoneRedundancyFourFullCopiesEnforceUniqueZones() throws Unkno
     assertThat(bucketOperator.removes).isEqualTo(expectedRemoves);
   }
 
-
+  /**
+   * Test that when you have five zones with one server each, with enforce 
unique zones, a
+   * redundancy
+   * level of 1, there is an overredundancy, and two servers have no data, 
that the overredundancy
+   * gets removed and buckets get moved to rebalance the data load
+   *
+   */
   @Test
-  public void testFiveZoneRedundancyFourFullCopiesEnforceUniqueZones() throws 
UnknownHostException {
+  public void fiveZonesWithRedundancyAndFourFullCopiesAndEnforceUniqueZones()

Review comment:
       This test name seems to be incorrect as there are three copies of each 
bucket, not four.

##########
File path: 
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/control/RebalanceOperationComplexPart2DistributedTest.java
##########
@@ -220,21 +209,13 @@ protected void doRebalance(ResourceManager manager)
   /**
    * Get the bucket count for the region in the redundancy zone
    *
-   * @param regionName - name of the region to get the bucket count of
-   * @param zoneName - redundancy zone for which to get the bucket count
    * @return - the total bucket count for the region in the redundancy zone
    */
-  protected int getZoneBucketCount(String regionName, String zoneName) {
+  protected int getZoneBucketCount() {

Review comment:
       This method name should probably be changed to "getZoneABucketCount()"




-- 
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]


Reply via email to