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 2021/09/07 20:52:18 UTC
[GitHub] [geode] mhansonp opened a new pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
mhansonp opened a new pull request #6845:
URL: https://github.com/apache/geode/pull/6845
If there is no redundancy zone, exit and allow delete
Adding xml files to git
Adding xml files to git
Added new tests
Move change from support/1.12
--
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] DonalEvans commented on a change in pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6845:
URL: https://github.com/apache/geode/pull/6845#discussion_r705699740
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/control/RebalanceOperationComplexDistributedTest.java
##########
@@ -0,0 +1,272 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.control;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.apache.geode.distributed.ConfigurationProperties.REDUNDANCY_ZONE;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.DEFAULT_DISK_DIRS_PROPERTY;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.GEODE_PREFIX;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.Serializable;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Stream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.apache.commons.io.FileUtils;
+import org.apache.logging.log4j.Logger;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.control.ResourceManager;
+import org.apache.geode.internal.cache.PartitionedRegion;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.ClientVM;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+
+/**
+ * The purpose of RebalanceOperationComplexDistributedTest is to test rebalances
+ * across zones and to ensure that enforceUniqueZone behavior of redundancy zones
+ * is working correctly.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class RebalanceOperationComplexDistributedTest implements Serializable {
+ public static final int EXPECTED_BUCKET_COUNT = 113;
+ public static final long TIMEOUT_SECONDS = GeodeAwaitility.getTimeout().getSeconds();
+ public static final String CLIENT_XML = "RebalanceOperationComplex-client.xml";
+ public static final String SERVER_XML = "RebalanceOperationComplex-server.xml";
+ public static final String REGION_NAME = "primary";
+ public static final String COLOCATED_REGION_NAME = "colocated";
+ public static final Logger logger = LogService.getLogger();
+
+ public static final String ZONE_A = "zoneA";
+ public static final String ZONE_B = "zoneB";
+ public int locatorPort;
+ public static final AtomicInteger runID = new AtomicInteger(0);
+ public String workingDir;
+
+ // 6 servers distributed evenly across 2 zones
+ public static final Map<Integer, String> SERVER_ZONE_MAP = new HashMap<Integer, String>() {
+ {
+ put(1, ZONE_A);
+ put(2, ZONE_A);
+ put(3, ZONE_A);
+ put(4, ZONE_B);
+ put(5, ZONE_B);
+ put(6, ZONE_B);
+ }
+ };
+
+ @Rule
+ public ClusterStartupRule clusterStartupRule = new ClusterStartupRule(8);
+
+ @Before
+ public void setup() {
+ // Start the locator
+ MemberVM locatorVM = clusterStartupRule.startLocatorVM(0);
+ locatorPort = locatorVM.getPort();
+
+ workingDir = clusterStartupRule.getWorkingDirRoot().getAbsolutePath();
+
+ runID.incrementAndGet();
+ cleanOutServerDirectories();
+ }
+
+ @After
+ public void after() {
+ stopServersAndDeleteDirectories();
Review comment:
Oof, that's a pain.
--
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] mhansonp commented on a change in pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6845:
URL: https://github.com/apache/geode/pull/6845#discussion_r705692317
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/control/RebalanceOperationComplexDistributedTest.java
##########
@@ -0,0 +1,272 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.control;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.apache.geode.distributed.ConfigurationProperties.REDUNDANCY_ZONE;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.DEFAULT_DISK_DIRS_PROPERTY;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.GEODE_PREFIX;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.Serializable;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Stream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.apache.commons.io.FileUtils;
+import org.apache.logging.log4j.Logger;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.control.ResourceManager;
+import org.apache.geode.internal.cache.PartitionedRegion;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.ClientVM;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+
+/**
+ * The purpose of RebalanceOperationComplexDistributedTest is to test rebalances
+ * across zones and to ensure that enforceUniqueZone behavior of redundancy zones
+ * is working correctly.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class RebalanceOperationComplexDistributedTest implements Serializable {
+ public static final int EXPECTED_BUCKET_COUNT = 113;
+ public static final long TIMEOUT_SECONDS = GeodeAwaitility.getTimeout().getSeconds();
+ public static final String CLIENT_XML = "RebalanceOperationComplex-client.xml";
+ public static final String SERVER_XML = "RebalanceOperationComplex-server.xml";
+ public static final String REGION_NAME = "primary";
+ public static final String COLOCATED_REGION_NAME = "colocated";
+ public static final Logger logger = LogService.getLogger();
+
+ public static final String ZONE_A = "zoneA";
+ public static final String ZONE_B = "zoneB";
+ public int locatorPort;
+ public static final AtomicInteger runID = new AtomicInteger(0);
+ public String workingDir;
+
+ // 6 servers distributed evenly across 2 zones
+ public static final Map<Integer, String> SERVER_ZONE_MAP = new HashMap<Integer, String>() {
+ {
+ put(1, ZONE_A);
+ put(2, ZONE_A);
+ put(3, ZONE_A);
+ put(4, ZONE_B);
+ put(5, ZONE_B);
+ put(6, ZONE_B);
+ }
+ };
+
+ @Rule
+ public ClusterStartupRule clusterStartupRule = new ClusterStartupRule(8);
+
+ @Before
+ public void setup() {
+ // Start the locator
+ MemberVM locatorVM = clusterStartupRule.startLocatorVM(0);
+ locatorPort = locatorVM.getPort();
+
+ workingDir = clusterStartupRule.getWorkingDirRoot().getAbsolutePath();
+
+ runID.incrementAndGet();
+ cleanOutServerDirectories();
+ }
+
+ @After
+ public void after() {
+ stopServersAndDeleteDirectories();
Review comment:
It doesn't do it right for my test. I have been down that road.
--
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] mhansonp commented on a change in pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6845:
URL: https://github.com/apache/geode/pull/6845#discussion_r705507169
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/PartitionedRegionLoadModel.java
##########
@@ -473,21 +478,23 @@ public Move findBestRemove(Bucket bucket) {
Move bestMove = null;
for (Member member : bucket.getMembersHosting()) {
- float newLoad = (member.getTotalLoad() - bucket.getLoad()) / member.getWeight();
- if (newLoad > mostLoaded && !member.equals(bucket.getPrimary())) {
- Move move = new Move(null, member, bucket);
- if (!this.attemptedBucketRemoves.contains(move)) {
- mostLoaded = newLoad;
- bestMove = move;
+ if (member.canDelete(bucket, partitionedRegion.getDistributionManager()).willAccept()) {
+ float newLoad = (member.getTotalLoad() - bucket.getLoad()) / member.getWeight();
+ if (newLoad > mostLoaded && !member.equals(bucket.getPrimary())) {
+ Move move = new Move(null, member, bucket);
+ if (!this.attemptedBucketRemoves.contains(move)) {
+ mostLoaded = newLoad;
+ bestMove = move;
+ }
Review comment:
Done
--
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] mhansonp commented on a change in pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6845:
URL: https://github.com/apache/geode/pull/6845#discussion_r705693676
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/control/RebalanceOperationComplexDistributedTest.java
##########
@@ -0,0 +1,272 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.control;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.apache.geode.distributed.ConfigurationProperties.REDUNDANCY_ZONE;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.DEFAULT_DISK_DIRS_PROPERTY;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.GEODE_PREFIX;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.Serializable;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Stream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.apache.commons.io.FileUtils;
+import org.apache.logging.log4j.Logger;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.control.ResourceManager;
+import org.apache.geode.internal.cache.PartitionedRegion;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.ClientVM;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+
+/**
+ * The purpose of RebalanceOperationComplexDistributedTest is to test rebalances
+ * across zones and to ensure that enforceUniqueZone behavior of redundancy zones
+ * is working correctly.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class RebalanceOperationComplexDistributedTest implements Serializable {
+ public static final int EXPECTED_BUCKET_COUNT = 113;
+ public static final long TIMEOUT_SECONDS = GeodeAwaitility.getTimeout().getSeconds();
+ public static final String CLIENT_XML = "RebalanceOperationComplex-client.xml";
+ public static final String SERVER_XML = "RebalanceOperationComplex-server.xml";
Review comment:
The xml is doing things I can't do in standard API without cheating.
--
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] mhansonp commented on a change in pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6845:
URL: https://github.com/apache/geode/pull/6845#discussion_r705689872
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/Member.java
##########
@@ -40,21 +43,73 @@
private final boolean isCritical;
private final boolean enforceLocalMaxMemory;
- Member(AddressComparor addressComparor, InternalDistributedMember memberId, boolean isCritical,
+ @VisibleForTesting
+ public Member(AddressComparor addressComparor, InternalDistributedMember memberId,
+ boolean isCritical,
boolean enforceLocalMaxMemory) {
this.addressComparor = addressComparor;
this.memberId = memberId;
this.isCritical = isCritical;
this.enforceLocalMaxMemory = enforceLocalMaxMemory;
}
- Member(AddressComparor addressComparor, InternalDistributedMember memberId, float weight,
+ @VisibleForTesting
+ public Member(AddressComparor addressComparor, InternalDistributedMember memberId, float weight,
long localMaxMemory, boolean isCritical, boolean enforceLocalMaxMemory) {
this(addressComparor, memberId, isCritical, enforceLocalMaxMemory);
this.weight = weight;
this.localMaxMemory = localMaxMemory;
}
+ /**
+ * Check to see if the member is the last copy of the bucket in the redundancy zone
+ *
+ * @param bucket -- bucket to be deleted from the member
+ * @param distributionManager -- used to check members of redundancy zones
+ */
+
+ public RefusalReason canDelete(Bucket bucket, DistributionManager distributionManager) {
+ // This code only applies to Clusters.
+ if (!(distributionManager instanceof ClusterDistributionManager)) {
+ return RefusalReason.NONE;
+ }
+
+ ClusterDistributionManager clstrDistrMgr = (ClusterDistributionManager) distributionManager;
Review comment:
Well, I needed that on 1.12 so I will see about cleaning that up.
--
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] mhansonp commented on a change in pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6845:
URL: https://github.com/apache/geode/pull/6845#discussion_r705693451
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/control/RebalanceOperationComplexDistributedTest.java
##########
@@ -0,0 +1,272 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.control;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.apache.geode.distributed.ConfigurationProperties.REDUNDANCY_ZONE;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.DEFAULT_DISK_DIRS_PROPERTY;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.GEODE_PREFIX;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.Serializable;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Stream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.apache.commons.io.FileUtils;
+import org.apache.logging.log4j.Logger;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.control.ResourceManager;
+import org.apache.geode.internal.cache.PartitionedRegion;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.ClientVM;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+
+/**
+ * The purpose of RebalanceOperationComplexDistributedTest is to test rebalances
+ * across zones and to ensure that enforceUniqueZone behavior of redundancy zones
+ * is working correctly.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class RebalanceOperationComplexDistributedTest implements Serializable {
+ public static final int EXPECTED_BUCKET_COUNT = 113;
+ public static final long TIMEOUT_SECONDS = GeodeAwaitility.getTimeout().getSeconds();
+ public static final String CLIENT_XML = "RebalanceOperationComplex-client.xml";
+ public static final String SERVER_XML = "RebalanceOperationComplex-server.xml";
+ public static final String REGION_NAME = "primary";
+ public static final String COLOCATED_REGION_NAME = "colocated";
+ public static final Logger logger = LogService.getLogger();
+
+ public static final String ZONE_A = "zoneA";
+ public static final String ZONE_B = "zoneB";
+ public int locatorPort;
+ public static final AtomicInteger runID = new AtomicInteger(0);
+ public String workingDir;
+
+ // 6 servers distributed evenly across 2 zones
+ public static final Map<Integer, String> SERVER_ZONE_MAP = new HashMap<Integer, String>() {
+ {
+ put(1, ZONE_A);
+ put(2, ZONE_A);
+ put(3, ZONE_A);
+ put(4, ZONE_B);
+ put(5, ZONE_B);
+ put(6, ZONE_B);
+ }
+ };
+
+ @Rule
+ public ClusterStartupRule clusterStartupRule = new ClusterStartupRule(8);
+
+ @Before
+ public void setup() {
+ // Start the locator
+ MemberVM locatorVM = clusterStartupRule.startLocatorVM(0);
+ locatorPort = locatorVM.getPort();
+
+ workingDir = clusterStartupRule.getWorkingDirRoot().getAbsolutePath();
+
+ runID.incrementAndGet();
+ cleanOutServerDirectories();
Review comment:
It is necessary because I cannot use DistributedDiskRule with ClusterStartupRule and have it behave consistently.
--
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] mhansonp commented on a change in pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6845:
URL: https://github.com/apache/geode/pull/6845#discussion_r705691727
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/Member.java
##########
@@ -40,21 +43,73 @@
private final boolean isCritical;
private final boolean enforceLocalMaxMemory;
- Member(AddressComparor addressComparor, InternalDistributedMember memberId, boolean isCritical,
+ @VisibleForTesting
+ public Member(AddressComparor addressComparor, InternalDistributedMember memberId,
+ boolean isCritical,
boolean enforceLocalMaxMemory) {
this.addressComparor = addressComparor;
this.memberId = memberId;
this.isCritical = isCritical;
this.enforceLocalMaxMemory = enforceLocalMaxMemory;
}
- Member(AddressComparor addressComparor, InternalDistributedMember memberId, float weight,
+ @VisibleForTesting
+ public Member(AddressComparor addressComparor, InternalDistributedMember memberId, float weight,
long localMaxMemory, boolean isCritical, boolean enforceLocalMaxMemory) {
this(addressComparor, memberId, isCritical, enforceLocalMaxMemory);
this.weight = weight;
this.localMaxMemory = localMaxMemory;
}
+ /**
+ * Check to see if the member is the last copy of the bucket in the redundancy zone
+ *
+ * @param bucket -- bucket to be deleted from the member
+ * @param distributionManager -- used to check members of redundancy zones
+ */
+
+ public RefusalReason canDelete(Bucket bucket, DistributionManager distributionManager) {
+ // This code only applies to Clusters.
+ if (!(distributionManager instanceof ClusterDistributionManager)) {
+ return RefusalReason.NONE;
+ }
+
+ ClusterDistributionManager clstrDistrMgr = (ClusterDistributionManager) distributionManager;
+ String myRedundancyZone = clstrDistrMgr.getRedundancyZone(memberId);
+ boolean lastMemberOfZone = true;
+
+ if (myRedundancyZone == null) {
+ // Not using redundancy zones, so...
+ return RefusalReason.NONE;
+ }
+
+ for (Member member : bucket.getMembersHosting()) {
+ // Don't look at yourself because you are not redundant for yourself
+ if (member.getMemberId().equals(this.getMemberId())) {
+ continue;
+ }
+
+ String memberRedundancyZone = clstrDistrMgr.getRedundancyZone(member.memberId);
+ if (memberRedundancyZone == null) {
+ // Not using redundancy zones, so...
+ continue;
+ }
+
+ // Does the member redundancy zone match my redundancy zone?
+ // if so we are not the last in the redundancy zone.
+ if (memberRedundancyZone.equals(myRedundancyZone)) {
+ lastMemberOfZone = false;
Review comment:
good suggestion
--
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] mhansonp commented on a change in pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6845:
URL: https://github.com/apache/geode/pull/6845#discussion_r705692648
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/control/RebalanceOperationComplexDistributedTest.java
##########
@@ -0,0 +1,272 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.control;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.apache.geode.distributed.ConfigurationProperties.REDUNDANCY_ZONE;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.DEFAULT_DISK_DIRS_PROPERTY;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.GEODE_PREFIX;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.Serializable;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Stream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.apache.commons.io.FileUtils;
+import org.apache.logging.log4j.Logger;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.control.ResourceManager;
+import org.apache.geode.internal.cache.PartitionedRegion;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.ClientVM;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+
+/**
+ * The purpose of RebalanceOperationComplexDistributedTest is to test rebalances
+ * across zones and to ensure that enforceUniqueZone behavior of redundancy zones
+ * is working correctly.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class RebalanceOperationComplexDistributedTest implements Serializable {
+ public static final int EXPECTED_BUCKET_COUNT = 113;
+ public static final long TIMEOUT_SECONDS = GeodeAwaitility.getTimeout().getSeconds();
+ public static final String CLIENT_XML = "RebalanceOperationComplex-client.xml";
+ public static final String SERVER_XML = "RebalanceOperationComplex-server.xml";
+ public static final String REGION_NAME = "primary";
+ public static final String COLOCATED_REGION_NAME = "colocated";
+ public static final Logger logger = LogService.getLogger();
+
+ public static final String ZONE_A = "zoneA";
+ public static final String ZONE_B = "zoneB";
+ public int locatorPort;
+ public static final AtomicInteger runID = new AtomicInteger(0);
+ public String workingDir;
+
+ // 6 servers distributed evenly across 2 zones
+ public static final Map<Integer, String> SERVER_ZONE_MAP = new HashMap<Integer, String>() {
+ {
+ put(1, ZONE_A);
+ put(2, ZONE_A);
+ put(3, ZONE_A);
+ put(4, ZONE_B);
+ put(5, ZONE_B);
+ put(6, ZONE_B);
+ }
+ };
+
+ @Rule
+ public ClusterStartupRule clusterStartupRule = new ClusterStartupRule(8);
+
+ @Before
+ public void setup() {
+ // Start the locator
+ MemberVM locatorVM = clusterStartupRule.startLocatorVM(0);
+ locatorPort = locatorVM.getPort();
+
+ workingDir = clusterStartupRule.getWorkingDirRoot().getAbsolutePath();
+
+ runID.incrementAndGet();
+ cleanOutServerDirectories();
+ }
+
+ @After
+ public void after() {
+ stopServersAndDeleteDirectories();
Review comment:
The diskdir rule does, but it doesn't interact consistently with the ClusterStartupRule.
--
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] mhansonp merged pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
mhansonp merged pull request #6845:
URL: https://github.com/apache/geode/pull/6845
--
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] mhansonp commented on a change in pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6845:
URL: https://github.com/apache/geode/pull/6845#discussion_r705691727
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/Member.java
##########
@@ -40,21 +43,73 @@
private final boolean isCritical;
private final boolean enforceLocalMaxMemory;
- Member(AddressComparor addressComparor, InternalDistributedMember memberId, boolean isCritical,
+ @VisibleForTesting
+ public Member(AddressComparor addressComparor, InternalDistributedMember memberId,
+ boolean isCritical,
boolean enforceLocalMaxMemory) {
this.addressComparor = addressComparor;
this.memberId = memberId;
this.isCritical = isCritical;
this.enforceLocalMaxMemory = enforceLocalMaxMemory;
}
- Member(AddressComparor addressComparor, InternalDistributedMember memberId, float weight,
+ @VisibleForTesting
+ public Member(AddressComparor addressComparor, InternalDistributedMember memberId, float weight,
long localMaxMemory, boolean isCritical, boolean enforceLocalMaxMemory) {
this(addressComparor, memberId, isCritical, enforceLocalMaxMemory);
this.weight = weight;
this.localMaxMemory = localMaxMemory;
}
+ /**
+ * Check to see if the member is the last copy of the bucket in the redundancy zone
+ *
+ * @param bucket -- bucket to be deleted from the member
+ * @param distributionManager -- used to check members of redundancy zones
+ */
+
+ public RefusalReason canDelete(Bucket bucket, DistributionManager distributionManager) {
+ // This code only applies to Clusters.
+ if (!(distributionManager instanceof ClusterDistributionManager)) {
+ return RefusalReason.NONE;
+ }
+
+ ClusterDistributionManager clstrDistrMgr = (ClusterDistributionManager) distributionManager;
+ String myRedundancyZone = clstrDistrMgr.getRedundancyZone(memberId);
+ boolean lastMemberOfZone = true;
+
+ if (myRedundancyZone == null) {
+ // Not using redundancy zones, so...
+ return RefusalReason.NONE;
+ }
+
+ for (Member member : bucket.getMembersHosting()) {
+ // Don't look at yourself because you are not redundant for yourself
+ if (member.getMemberId().equals(this.getMemberId())) {
+ continue;
+ }
+
+ String memberRedundancyZone = clstrDistrMgr.getRedundancyZone(member.memberId);
+ if (memberRedundancyZone == null) {
+ // Not using redundancy zones, so...
+ continue;
+ }
+
+ // Does the member redundancy zone match my redundancy zone?
+ // if so we are not the last in the redundancy zone.
+ if (memberRedundancyZone.equals(myRedundancyZone)) {
+ lastMemberOfZone = false;
Review comment:
Good suggestion!
--
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] jchen21 commented on a change in pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #6845:
URL: https://github.com/apache/geode/pull/6845#discussion_r703925971
##########
File path: geode-core/src/distributedTest/resources/org/apache/geode/internal/cache/RebalanceOperationComplex-client.xml
##########
@@ -0,0 +1,33 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<!DOCTYPE client-cache PUBLIC
Review comment:
I have the same comment as those in the pull request you closed #6839.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/control/PartitionRebalanceDetailsImpl.java
##########
@@ -48,6 +48,27 @@
private long time;
private int numOfMembers;
+ @Override
+ public String toString() {
Review comment:
I have the same comment as those in the pull request you closed #6839.
##########
File path: geode-core/src/distributedTest/resources/org/apache/geode/internal/cache/RebalanceOperationComplex-client.xml
##########
@@ -0,0 +1,33 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<!DOCTYPE client-cache PUBLIC
+ "-//GemStone Systems, Inc.//GemFire Declarative Caching 7.0//EN"
+ "http://www.gemstone.com/dtd/cache7_0.dtd">
+
+<client-cache>
+ <pdx read-serialized="true">
Review comment:
I have the same comment as those in the pull request you closed #6839.
--
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] kirklund commented on a change in pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6845:
URL: https://github.com/apache/geode/pull/6845#discussion_r703851825
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/rebalance/MemberTest.java
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.partitioned.rebalance;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.junit.Test;
+import org.mockito.ArgumentMatchers;
+import org.mockito.Mockito;
+
+import org.apache.geode.distributed.internal.ClusterDistributionManager;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.partitioned.rebalance.model.AddressComparor;
+import org.apache.geode.internal.cache.partitioned.rebalance.model.Bucket;
+import org.apache.geode.internal.cache.partitioned.rebalance.model.Member;
+import org.apache.geode.internal.cache.partitioned.rebalance.model.RefusalReason;
+
+public class MemberTest {
+ AddressComparor addressComparor = mock(AddressComparor.class);
+ InternalDistributedMember memberId = mock(InternalDistributedMember.class);
+ InternalDistributedMember otherMemberId = mock(InternalDistributedMember.class);
Review comment:
You should probably make fields like these `private` even though it's unlikely someone will try to access them. I only add this because I've seen so many tangled dunit tests that access each other's non-private fields.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/PartitionedRegionLoadModel.java
##########
@@ -473,21 +478,23 @@ public Move findBestRemove(Bucket bucket) {
Move bestMove = null;
for (Member member : bucket.getMembersHosting()) {
- float newLoad = (member.getTotalLoad() - bucket.getLoad()) / member.getWeight();
- if (newLoad > mostLoaded && !member.equals(bucket.getPrimary())) {
- Move move = new Move(null, member, bucket);
- if (!this.attemptedBucketRemoves.contains(move)) {
- mostLoaded = newLoad;
- bestMove = move;
+ if (member.canDelete(bucket, partitionedRegion.getDistributionManager()).willAccept()) {
+ float newLoad = (member.getTotalLoad() - bucket.getLoad()) / member.getWeight();
+ if (newLoad > mostLoaded && !member.equals(bucket.getPrimary())) {
+ Move move = new Move(null, member, bucket);
+ if (!this.attemptedBucketRemoves.contains(move)) {
+ mostLoaded = newLoad;
+ bestMove = move;
+ }
Review comment:
I would add a unit test for this block if possible.
--
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] mhansonp commented on a change in pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6845:
URL: https://github.com/apache/geode/pull/6845#discussion_r704894638
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/control/PartitionRebalanceDetailsImpl.java
##########
@@ -48,6 +48,27 @@
private long time;
private int numOfMembers;
+ @Override
+ public String toString() {
Review comment:
Actually, I have addressed all of your concerns except the toString and numOfMembers. As I mention (after your comment) I don't think numOfMembers is required for my implementation, so I am hesitant to do anything with it. I will update the toString though.
--
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] DonalEvans commented on a change in pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6845:
URL: https://github.com/apache/geode/pull/6845#discussion_r705610747
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/Member.java
##########
@@ -40,21 +43,73 @@
private final boolean isCritical;
private final boolean enforceLocalMaxMemory;
- Member(AddressComparor addressComparor, InternalDistributedMember memberId, boolean isCritical,
+ @VisibleForTesting
+ public Member(AddressComparor addressComparor, InternalDistributedMember memberId,
+ boolean isCritical,
boolean enforceLocalMaxMemory) {
this.addressComparor = addressComparor;
this.memberId = memberId;
this.isCritical = isCritical;
this.enforceLocalMaxMemory = enforceLocalMaxMemory;
}
- Member(AddressComparor addressComparor, InternalDistributedMember memberId, float weight,
+ @VisibleForTesting
+ public Member(AddressComparor addressComparor, InternalDistributedMember memberId, float weight,
long localMaxMemory, boolean isCritical, boolean enforceLocalMaxMemory) {
this(addressComparor, memberId, isCritical, enforceLocalMaxMemory);
this.weight = weight;
this.localMaxMemory = localMaxMemory;
}
+ /**
+ * Check to see if the member is the last copy of the bucket in the redundancy zone
+ *
+ * @param bucket -- bucket to be deleted from the member
+ * @param distributionManager -- used to check members of redundancy zones
+ */
+
+ public RefusalReason canDelete(Bucket bucket, DistributionManager distributionManager) {
+ // This code only applies to Clusters.
+ if (!(distributionManager instanceof ClusterDistributionManager)) {
+ return RefusalReason.NONE;
+ }
+
+ ClusterDistributionManager clstrDistrMgr = (ClusterDistributionManager) distributionManager;
Review comment:
This cast is unnecessary, as the methods called on the distribution manager exist on the interface.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/control/RebalanceOperationComplexDistributedTest.java
##########
@@ -0,0 +1,272 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.control;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.apache.geode.distributed.ConfigurationProperties.REDUNDANCY_ZONE;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.DEFAULT_DISK_DIRS_PROPERTY;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.GEODE_PREFIX;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.Serializable;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Stream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.apache.commons.io.FileUtils;
+import org.apache.logging.log4j.Logger;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.control.ResourceManager;
+import org.apache.geode.internal.cache.PartitionedRegion;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.ClientVM;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+
+/**
+ * The purpose of RebalanceOperationComplexDistributedTest is to test rebalances
+ * across zones and to ensure that enforceUniqueZone behavior of redundancy zones
+ * is working correctly.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class RebalanceOperationComplexDistributedTest implements Serializable {
+ public static final int EXPECTED_BUCKET_COUNT = 113;
+ public static final long TIMEOUT_SECONDS = GeodeAwaitility.getTimeout().getSeconds();
+ public static final String CLIENT_XML = "RebalanceOperationComplex-client.xml";
+ public static final String SERVER_XML = "RebalanceOperationComplex-server.xml";
+ public static final String REGION_NAME = "primary";
+ public static final String COLOCATED_REGION_NAME = "colocated";
+ public static final Logger logger = LogService.getLogger();
+
+ public static final String ZONE_A = "zoneA";
+ public static final String ZONE_B = "zoneB";
+ public int locatorPort;
+ public static final AtomicInteger runID = new AtomicInteger(0);
+ public String workingDir;
+
+ // 6 servers distributed evenly across 2 zones
+ public static final Map<Integer, String> SERVER_ZONE_MAP = new HashMap<Integer, String>() {
+ {
+ put(1, ZONE_A);
+ put(2, ZONE_A);
+ put(3, ZONE_A);
+ put(4, ZONE_B);
+ put(5, ZONE_B);
+ put(6, ZONE_B);
+ }
+ };
+
+ @Rule
+ public ClusterStartupRule clusterStartupRule = new ClusterStartupRule(8);
+
+ @Before
+ public void setup() {
+ // Start the locator
+ MemberVM locatorVM = clusterStartupRule.startLocatorVM(0);
+ locatorPort = locatorVM.getPort();
+
+ workingDir = clusterStartupRule.getWorkingDirRoot().getAbsolutePath();
+
+ runID.incrementAndGet();
+ cleanOutServerDirectories();
+ }
+
+ @After
+ public void after() {
+ stopServersAndDeleteDirectories();
+ }
+
+ /**
+ * Test that we correctly use the redundancy-zone property to determine where to place redundant
+ * copies of a buckets and doesn't allow cross redundancy zone deletes.
+ *
+ * @param rebalanceServer - the server index that will initiate all the rebalances
+ * @param serverToBeShutdownAndRestarted - the server index that will be shutdown and restarted
+ */
+ @Test
+ @Parameters({"1,2", "1,4", "4,1", "5,6"})
+ public void testEnforceZoneWithSixServersAndTwoZones(int rebalanceServer,
+ int serverToBeShutdownAndRestarted)
+ throws Exception {
+
+ // Startup the servers
+ for (Map.Entry<Integer, String> entry : SERVER_ZONE_MAP.entrySet()) {
+ startServerInRedundancyZone(entry.getKey(), entry.getValue());
+ }
+
+ // Put data in the server regions
+ clientPopulateServers();
Review comment:
This could probably be moved to the `setUp()` method.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/control/RebalanceOperationComplexDistributedTest.java
##########
@@ -0,0 +1,272 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.control;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.apache.geode.distributed.ConfigurationProperties.REDUNDANCY_ZONE;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.DEFAULT_DISK_DIRS_PROPERTY;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.GEODE_PREFIX;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.Serializable;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Stream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.apache.commons.io.FileUtils;
+import org.apache.logging.log4j.Logger;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.control.ResourceManager;
+import org.apache.geode.internal.cache.PartitionedRegion;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.ClientVM;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+
+/**
+ * The purpose of RebalanceOperationComplexDistributedTest is to test rebalances
+ * across zones and to ensure that enforceUniqueZone behavior of redundancy zones
+ * is working correctly.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class RebalanceOperationComplexDistributedTest implements Serializable {
+ public static final int EXPECTED_BUCKET_COUNT = 113;
+ public static final long TIMEOUT_SECONDS = GeodeAwaitility.getTimeout().getSeconds();
+ public static final String CLIENT_XML = "RebalanceOperationComplex-client.xml";
+ public static final String SERVER_XML = "RebalanceOperationComplex-server.xml";
+ public static final String REGION_NAME = "primary";
+ public static final String COLOCATED_REGION_NAME = "colocated";
+ public static final Logger logger = LogService.getLogger();
+
+ public static final String ZONE_A = "zoneA";
+ public static final String ZONE_B = "zoneB";
+ public int locatorPort;
+ public static final AtomicInteger runID = new AtomicInteger(0);
+ public String workingDir;
+
+ // 6 servers distributed evenly across 2 zones
+ public static final Map<Integer, String> SERVER_ZONE_MAP = new HashMap<Integer, String>() {
+ {
+ put(1, ZONE_A);
+ put(2, ZONE_A);
+ put(3, ZONE_A);
+ put(4, ZONE_B);
+ put(5, ZONE_B);
+ put(6, ZONE_B);
+ }
+ };
+
+ @Rule
+ public ClusterStartupRule clusterStartupRule = new ClusterStartupRule(8);
+
+ @Before
+ public void setup() {
+ // Start the locator
+ MemberVM locatorVM = clusterStartupRule.startLocatorVM(0);
+ locatorPort = locatorVM.getPort();
+
+ workingDir = clusterStartupRule.getWorkingDirRoot().getAbsolutePath();
+
+ runID.incrementAndGet();
+ cleanOutServerDirectories();
+ }
+
+ @After
+ public void after() {
+ stopServersAndDeleteDirectories();
Review comment:
I think this may be unnecessary, as `ClusterStartupRule` takes care of this clean-up.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/rebalance/model/Member.java
##########
@@ -40,21 +43,73 @@
private final boolean isCritical;
private final boolean enforceLocalMaxMemory;
- Member(AddressComparor addressComparor, InternalDistributedMember memberId, boolean isCritical,
+ @VisibleForTesting
+ public Member(AddressComparor addressComparor, InternalDistributedMember memberId,
+ boolean isCritical,
boolean enforceLocalMaxMemory) {
this.addressComparor = addressComparor;
this.memberId = memberId;
this.isCritical = isCritical;
this.enforceLocalMaxMemory = enforceLocalMaxMemory;
}
- Member(AddressComparor addressComparor, InternalDistributedMember memberId, float weight,
+ @VisibleForTesting
+ public Member(AddressComparor addressComparor, InternalDistributedMember memberId, float weight,
long localMaxMemory, boolean isCritical, boolean enforceLocalMaxMemory) {
this(addressComparor, memberId, isCritical, enforceLocalMaxMemory);
this.weight = weight;
this.localMaxMemory = localMaxMemory;
}
+ /**
+ * Check to see if the member is the last copy of the bucket in the redundancy zone
+ *
+ * @param bucket -- bucket to be deleted from the member
+ * @param distributionManager -- used to check members of redundancy zones
+ */
+
+ public RefusalReason canDelete(Bucket bucket, DistributionManager distributionManager) {
+ // This code only applies to Clusters.
+ if (!(distributionManager instanceof ClusterDistributionManager)) {
+ return RefusalReason.NONE;
+ }
+
+ ClusterDistributionManager clstrDistrMgr = (ClusterDistributionManager) distributionManager;
+ String myRedundancyZone = clstrDistrMgr.getRedundancyZone(memberId);
+ boolean lastMemberOfZone = true;
+
+ if (myRedundancyZone == null) {
+ // Not using redundancy zones, so...
+ return RefusalReason.NONE;
+ }
+
+ for (Member member : bucket.getMembersHosting()) {
+ // Don't look at yourself because you are not redundant for yourself
+ if (member.getMemberId().equals(this.getMemberId())) {
+ continue;
+ }
+
+ String memberRedundancyZone = clstrDistrMgr.getRedundancyZone(member.memberId);
+ if (memberRedundancyZone == null) {
+ // Not using redundancy zones, so...
+ continue;
+ }
+
+ // Does the member redundancy zone match my redundancy zone?
+ // if so we are not the last in the redundancy zone.
+ if (memberRedundancyZone.equals(myRedundancyZone)) {
+ lastMemberOfZone = false;
Review comment:
Here we could instead just `return RefusalReason.NONE;` and eliminate the `lastMemberOfZone` variable, since it will always be true if we exit the loop without hitting this line.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/control/RebalanceOperationComplexDistributedTest.java
##########
@@ -0,0 +1,272 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.control;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.apache.geode.distributed.ConfigurationProperties.REDUNDANCY_ZONE;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.DEFAULT_DISK_DIRS_PROPERTY;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.GEODE_PREFIX;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.Serializable;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Stream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.apache.commons.io.FileUtils;
+import org.apache.logging.log4j.Logger;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.control.ResourceManager;
+import org.apache.geode.internal.cache.PartitionedRegion;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.ClientVM;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+
+/**
+ * The purpose of RebalanceOperationComplexDistributedTest is to test rebalances
+ * across zones and to ensure that enforceUniqueZone behavior of redundancy zones
+ * is working correctly.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class RebalanceOperationComplexDistributedTest implements Serializable {
+ public static final int EXPECTED_BUCKET_COUNT = 113;
+ public static final long TIMEOUT_SECONDS = GeodeAwaitility.getTimeout().getSeconds();
+ public static final String CLIENT_XML = "RebalanceOperationComplex-client.xml";
+ public static final String SERVER_XML = "RebalanceOperationComplex-server.xml";
Review comment:
Is there a specific reason for this test to be using cache.xml to setup the client and servers? I believe it should be possible to achieve the same results using API calls or GFSH, which are more commonly used.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/control/RebalanceOperationComplexDistributedTest.java
##########
@@ -0,0 +1,272 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.control;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.apache.geode.distributed.ConfigurationProperties.REDUNDANCY_ZONE;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.DEFAULT_DISK_DIRS_PROPERTY;
+import static org.apache.geode.internal.lang.SystemPropertyHelper.GEODE_PREFIX;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.Serializable;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Stream;
+
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import org.apache.commons.io.FileUtils;
+import org.apache.logging.log4j.Logger;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.client.ClientCache;
+import org.apache.geode.cache.control.ResourceManager;
+import org.apache.geode.internal.cache.PartitionedRegion;
+import org.apache.geode.logging.internal.log4j.api.LogService;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.ClientVM;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+
+/**
+ * The purpose of RebalanceOperationComplexDistributedTest is to test rebalances
+ * across zones and to ensure that enforceUniqueZone behavior of redundancy zones
+ * is working correctly.
+ */
+@RunWith(JUnitParamsRunner.class)
+public class RebalanceOperationComplexDistributedTest implements Serializable {
+ public static final int EXPECTED_BUCKET_COUNT = 113;
+ public static final long TIMEOUT_SECONDS = GeodeAwaitility.getTimeout().getSeconds();
+ public static final String CLIENT_XML = "RebalanceOperationComplex-client.xml";
+ public static final String SERVER_XML = "RebalanceOperationComplex-server.xml";
+ public static final String REGION_NAME = "primary";
+ public static final String COLOCATED_REGION_NAME = "colocated";
+ public static final Logger logger = LogService.getLogger();
+
+ public static final String ZONE_A = "zoneA";
+ public static final String ZONE_B = "zoneB";
+ public int locatorPort;
+ public static final AtomicInteger runID = new AtomicInteger(0);
+ public String workingDir;
+
+ // 6 servers distributed evenly across 2 zones
+ public static final Map<Integer, String> SERVER_ZONE_MAP = new HashMap<Integer, String>() {
+ {
+ put(1, ZONE_A);
+ put(2, ZONE_A);
+ put(3, ZONE_A);
+ put(4, ZONE_B);
+ put(5, ZONE_B);
+ put(6, ZONE_B);
+ }
+ };
+
+ @Rule
+ public ClusterStartupRule clusterStartupRule = new ClusterStartupRule(8);
+
+ @Before
+ public void setup() {
+ // Start the locator
+ MemberVM locatorVM = clusterStartupRule.startLocatorVM(0);
+ locatorPort = locatorVM.getPort();
+
+ workingDir = clusterStartupRule.getWorkingDirRoot().getAbsolutePath();
+
+ runID.incrementAndGet();
+ cleanOutServerDirectories();
Review comment:
Why is this method necessary?
--
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] mhansonp commented on a change in pull request #6845: GEODE-9554: Change up the rebalance calls to use new canDelete call
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6845:
URL: https://github.com/apache/geode/pull/6845#discussion_r703916216
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/rebalance/MemberTest.java
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.partitioned.rebalance;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.junit.Test;
+import org.mockito.ArgumentMatchers;
+import org.mockito.Mockito;
+
+import org.apache.geode.distributed.internal.ClusterDistributionManager;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.partitioned.rebalance.model.AddressComparor;
+import org.apache.geode.internal.cache.partitioned.rebalance.model.Bucket;
+import org.apache.geode.internal.cache.partitioned.rebalance.model.Member;
+import org.apache.geode.internal.cache.partitioned.rebalance.model.RefusalReason;
+
+public class MemberTest {
+ AddressComparor addressComparor = mock(AddressComparor.class);
+ InternalDistributedMember memberId = mock(InternalDistributedMember.class);
+ InternalDistributedMember otherMemberId = mock(InternalDistributedMember.class);
Review comment:
done.
--
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