This is an automated email from the ASF dual-hosted git repository.
dschneider pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new ef533fa GEODE-7825: improve rebalance result (#4803)
ef533fa is described below
commit ef533fa839455d3b720facb8487d8e8e1e56c089
Author: Darrel Schneider <[email protected]>
AuthorDate: Mon Mar 16 11:20:46 2020 -0700
GEODE-7825: improve rebalance result (#4803)
* the statusMessage will now also contain the name of the exception
* OperationResult getSuccess and getStatusMessage are now used
to set the status and message on ClusterManagementOperationResult.
* Instead of throwing NoMembersException, the rebalance code
now sets the statusMessage on the RebalanceResult like it
already did for other exceptions. This will prevent a call
stack from being displayed to users when no member is found
to do the rebalance on.
---
.../management/internal/BaseManagementService.java | 9 ++++
.../api/LocatorClusterManagementService.java | 9 +++-
.../operation/RebalanceOperationPerformer.java | 8 +--
.../api/LocatorClusterManagementServiceTest.java | 63 ++++++++++++++++++++--
.../operation/RebalanceOperationPerformerTest.java | 19 +++++++
5 files changed, 98 insertions(+), 10 deletions(-)
diff --git
a/geode-core/src/main/java/org/apache/geode/management/internal/BaseManagementService.java
b/geode-core/src/main/java/org/apache/geode/management/internal/BaseManagementService.java
index 71d6b0a..c3f538f 100755
---
a/geode-core/src/main/java/org/apache/geode/management/internal/BaseManagementService.java
+++
b/geode-core/src/main/java/org/apache/geode/management/internal/BaseManagementService.java
@@ -21,6 +21,7 @@ import java.util.Map;
import org.apache.logging.log4j.Logger;
+import org.apache.geode.annotations.VisibleForTesting;
import org.apache.geode.annotations.internal.MakeNotStatic;
import org.apache.geode.distributed.DistributedSystemDisconnectedException;
import org.apache.geode.distributed.internal.InternalDistributedSystem;
@@ -85,6 +86,14 @@ public abstract class BaseManagementService extends
ManagementService {
}
}
+ @VisibleForTesting
+ public static void setManagementService(InternalCacheForClientAccess cache,
+ BaseManagementService service) {
+ synchronized (instances) {
+ instances.put(cache, service);
+ }
+ }
+
public static ManagementService getExistingManagementService(InternalCache
cache) {
synchronized (instances) {
BaseManagementService service =
instances.get(cache.getCacheForProcessingClientRequests());
diff --git
a/geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
b/geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
index 546042f..55605ac 100644
---
a/geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
+++
b/geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java
@@ -496,7 +496,14 @@ public class LocatorClusterManagementService implements
ClusterManagementService
resultStatus = StatusCode.IN_PROGRESS;
} else if (operationState.getThrowable() != null) {
resultStatus = StatusCode.ERROR;
- resultMessage = operationState.getThrowable().getMessage();
+ resultMessage = operationState.getThrowable().toString();
+ } else if (operationState.getResult() != null) {
+ if (!operationState.getResult().getSuccess()) {
+ resultStatus = StatusCode.ERROR;
+ }
+ if (operationState.getResult().getStatusMessage() != null) {
+ resultMessage = operationState.getResult().getStatusMessage();
+ }
}
return toClusterManagementOperationResult(resultStatus, resultMessage,
operationState);
}
diff --git
a/geode-core/src/main/java/org/apache/geode/management/internal/operation/RebalanceOperationPerformer.java
b/geode-core/src/main/java/org/apache/geode/management/internal/operation/RebalanceOperationPerformer.java
index f9e8463..ff14555 100644
---
a/geode-core/src/main/java/org/apache/geode/management/internal/operation/RebalanceOperationPerformer.java
+++
b/geode-core/src/main/java/org/apache/geode/management/internal/operation/RebalanceOperationPerformer.java
@@ -62,7 +62,7 @@ public class RebalanceOperationPerformer {
List<RebalanceRegionResult> rebalanceRegionResults = new ArrayList<>();
- RuntimeException latestNoMembersException = null;
+ NoMembersException latestNoMembersException = null;
for (String regionName : includeRegions) {
@@ -82,11 +82,11 @@ public class RebalanceOperationPerformer {
}
if (latestNoMembersException != null && !result.getSuccess()) {
- throw latestNoMembersException;
+ result.setStatusMessage(latestNoMembersException.getMessage());
+ } else {
+ result.setRebalanceSummary(rebalanceRegionResults);
}
- result.setRebalanceSummary(rebalanceRegionResults);
-
return result;
} else {
result =
diff --git
a/geode-core/src/test/java/org/apache/geode/management/internal/api/LocatorClusterManagementServiceTest.java
b/geode-core/src/test/java/org/apache/geode/management/internal/api/LocatorClusterManagementServiceTest.java
index 3120ce2..5a6f543 100644
---
a/geode-core/src/test/java/org/apache/geode/management/internal/api/LocatorClusterManagementServiceTest.java
+++
b/geode-core/src/test/java/org/apache/geode/management/internal/api/LocatorClusterManagementServiceTest.java
@@ -95,7 +95,7 @@ public class LocatorClusterManagementServiceTest {
private ClusterManagementResult result;
private Map<Class, ConfigurationValidator> validators = new HashMap<>();
private Map<Class, ConfigurationManager> managers = new HashMap<>();
- private OperationManager executorManager;
+ private OperationManager operationManager;
private ConfigurationValidator<Region> regionValidator;
private CommonConfigurationValidator cacheElementValidator;
private CacheConfigurationManager<Region> regionManager;
@@ -124,10 +124,10 @@ public class LocatorClusterManagementServiceTest {
doReturn(new CacheConfig()).when(persistenceService).getCacheConfig(any(),
anyBoolean());
doReturn(true).when(persistenceService).lockSharedConfiguration();
doNothing().when(persistenceService).unlockSharedConfiguration();
- executorManager = mock(OperationManager.class);
+ operationManager = mock(OperationManager.class);
service =
spy(new LocatorClusterManagementService(cache, persistenceService,
managers, validators,
- memberValidator, cacheElementValidator, executorManager));
+ memberValidator, cacheElementValidator, operationManager));
regionConfig = new Region();
regionConfig.setName("region1");
@@ -373,7 +373,7 @@ public class LocatorClusterManagementServiceTest {
final String URI = "/test/uri";
ClusterManagementOperation<OperationResult> operation =
mock(ClusterManagementOperation.class);
when(operation.getEndpoint()).thenReturn(URI);
- when(executorManager.submit(any()))
+ when(operationManager.submit(any()))
.thenReturn(new OperationState<>("42", operation, new Date()));
ClusterManagementOperationResult<?, ?> result = service.start(operation);
assertThat(result.getStatusCode()).isEqualTo(ClusterManagementResult.StatusCode.ACCEPTED);
@@ -389,7 +389,7 @@ public class LocatorClusterManagementServiceTest {
@Test
public void getRebalance() {
OperationState operationState = mock(OperationState.class);
- when(executorManager.get(any())).thenReturn(operationState);
+ when(operationManager.get(any())).thenReturn(operationState);
ClusterManagementOperationResult<RebalanceOperation, RebalanceResult>
result =
service.get(rebalanceOperation, "456");
assertThat(result.getStatusCode()).isEqualTo(ClusterManagementResult.StatusCode.IN_PROGRESS);
@@ -401,6 +401,59 @@ public class LocatorClusterManagementServiceTest {
}
@Test
+ public void
getRebalanceWithOperationStateThrowableCorrectlySetsStatusMessage() {
+ OperationState operationState = mock(OperationState.class);
+ when(operationManager.get(any())).thenReturn(operationState);
+ when(operationState.getOperationEnd()).thenReturn(new Date());
+ Throwable throwable = new NullPointerException("NPE");
+ when(operationState.getThrowable()).thenReturn(throwable);
+
+ ClusterManagementOperationResult<RebalanceOperation, RebalanceResult>
result =
+ service.get(rebalanceOperation, "456");
+
+
assertThat(result.getStatusCode()).isEqualTo(ClusterManagementResult.StatusCode.ERROR);
+ assertThat(result.getOperationResult()).isNull();
+ assertThat(result.getThrowable()).isSameAs(throwable);
+
assertThat(result.getStatusMessage()).contains("NPE").contains("NullPointerException");
+ }
+
+ @Test
+ public void
getRebalanceWithOperationResultThatFailedCorrectlySetsStatusMessage() {
+ OperationState operationState = mock(OperationState.class);
+ when(operationManager.get(any())).thenReturn(operationState);
+ when(operationState.getOperationEnd()).thenReturn(new Date());
+ OperationResult operationResult = mock(OperationResult.class);
+ when(operationResult.getSuccess()).thenReturn(false);
+ when(operationResult.getStatusMessage()).thenReturn("Failure status
message.");
+ when(operationState.getResult()).thenReturn(operationResult);
+
+ ClusterManagementOperationResult<RebalanceOperation, RebalanceResult>
result =
+ service.get(rebalanceOperation, "456");
+
+
assertThat(result.getStatusCode()).isEqualTo(ClusterManagementResult.StatusCode.ERROR);
+ assertThat(result.getOperationResult()).isSameAs(operationResult);
+ assertThat(result.getStatusMessage()).isEqualTo("Failure status message.");
+ }
+
+ @Test
+ public void
getRebalanceWithOperationResultThatSucceededCorrectlySetsStatusMessage() {
+ OperationState operationState = mock(OperationState.class);
+ when(operationManager.get(any())).thenReturn(operationState);
+ when(operationState.getOperationEnd()).thenReturn(new Date());
+ OperationResult operationResult = mock(OperationResult.class);
+ when(operationResult.getSuccess()).thenReturn(true);
+ when(operationResult.getStatusMessage()).thenReturn("Success status
message.");
+ when(operationState.getResult()).thenReturn(operationResult);
+
+ ClusterManagementOperationResult<RebalanceOperation, RebalanceResult>
result =
+ service.get(rebalanceOperation, "456");
+
+
assertThat(result.getStatusCode()).isEqualTo(ClusterManagementResult.StatusCode.OK);
+ assertThat(result.getOperationResult()).isSameAs(operationResult);
+ assertThat(result.getStatusMessage()).isEqualTo("Success status message.");
+ }
+
+ @Test
public void getRuntimeClass() {
assertSoftly(softly -> {
softly.assertThat(service.getRuntimeClass(Region.class)).isEqualTo(RuntimeRegionInfo.class);
diff --git
a/geode-core/src/test/java/org/apache/geode/management/internal/operation/RebalanceOperationPerformerTest.java
b/geode-core/src/test/java/org/apache/geode/management/internal/operation/RebalanceOperationPerformerTest.java
index 29f9856..baef301 100644
---
a/geode-core/src/test/java/org/apache/geode/management/internal/operation/RebalanceOperationPerformerTest.java
+++
b/geode-core/src/test/java/org/apache/geode/management/internal/operation/RebalanceOperationPerformerTest.java
@@ -33,9 +33,12 @@ import
org.apache.geode.distributed.internal.DistributionManager;
import org.apache.geode.distributed.internal.InternalDistributedSystem;
import
org.apache.geode.distributed.internal.membership.InternalDistributedMember;
import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.cache.InternalCacheForClientAccess;
import org.apache.geode.management.DistributedRegionMXBean;
import org.apache.geode.management.DistributedSystemMXBean;
import org.apache.geode.management.ManagementService;
+import org.apache.geode.management.internal.BaseManagementService;
+import org.apache.geode.management.operation.RebalanceOperation;
import org.apache.geode.management.runtime.RebalanceRegionResult;
import org.apache.geode.management.runtime.RebalanceResult;
@@ -143,7 +146,23 @@ public class RebalanceOperationPerformerTest {
assertThat(regionResult.getPrimaryTransferTimeInMilliseconds()).isEqualTo(6);
assertThat(regionResult.getPrimaryTransfersCompleted()).isEqualTo(7);
assertThat(regionResult.getTimeInMilliseconds()).isEqualTo(8);
+ }
+
+
+ @Test
+ public void
performWithIncludeRegionsWhenRegionOnNoMembersReturnsFalseWithCorrectMessage() {
+ RebalanceOperation rebalanceOperation = mock(RebalanceOperation.class);
+
when(rebalanceOperation.getIncludeRegions()).thenReturn(Collections.singletonList("region1"));
+ InternalCacheForClientAccess cache =
mock(InternalCacheForClientAccess.class);
+ when(cache.getCacheForProcessingClientRequests()).thenReturn(cache);
+ BaseManagementService managementService =
mock(BaseManagementService.class);
+ BaseManagementService.setManagementService(cache, managementService);
+
+ RebalanceResult result = RebalanceOperationPerformer.perform(cache,
rebalanceOperation);
+ assertThat(result.getSuccess()).isFalse();
+ assertThat(result.getStatusMessage())
+ .isEqualTo("For the region /region1, no member was found.");
}
}
