GEODE-6174: more error handling in LocatorClusterConfigurationService

geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/RegionManagementDunitTest.java

@@ -94,6 +94,13 @@ public void createsAPartitionedRegionByDefault() throws Exception {
 
     // make sure region is persisted
     locator.invoke(() -> verifyRegionPersisted("orders", "PARTITION"));
+
+    // create the same region 2nd time
+    result = restClient.doPostAndAssert("/regions", json)
+        .hasStatusCode(409)
+        .getClusterManagementResult();
+    assertThat(result.isSuccessfullyAppliedOnMembers()).isFalse();
+    assertThat(result.isSuccessfullyPersisted()).isFalse();
   }
 
   @Test

geode-assembly/src/integrationTest/java/org/apache/geode/management/internal/DisabledClusterConfigTest.java

@@ -0,0 +1,56 @@
+/*
+ * 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.management.internal;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.distributed.ConfigurationProperties;
+import org.apache.geode.management.internal.api.ClusterManagementResult;
+import org.apache.geode.test.junit.rules.GeodeDevRestClient;
+import org.apache.geode.test.junit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.rules.RequiresGeodeHome;
+
+public class DisabledClusterConfigTest {
+  @Rule
+  public LocatorStarterRule locator = new LocatorStarterRule();
+
+  @ClassRule
+  public static RequiresGeodeHome requiresGeodeHome = new RequiresGeodeHome();
+
+  public static GeodeDevRestClient restClient;
+
+  @Test
+  public void disabledClusterConfig() throws Exception {
+    locator.withProperty(ConfigurationProperties.ENABLE_CLUSTER_CONFIGURATION, "false")
+        .withHttpService().startLocator();
+    restClient =
+        new GeodeDevRestClient("/geode-management/v2", "localhost", locator.getHttpPort(), false);
+
+    ClusterManagementResult result =
+        restClient.doPostAndAssert("/regions", "{\"name\":\"test\"}")
+            .hasStatusCode(500)
+            .getClusterManagementResult();
+    assertThat(result.isSuccessful()).isFalse();
+    assertThat(result.isSuccessfullyPersisted()).isFalse();
+    assertThat(result.isSuccessfullyAppliedOnMembers()).isFalse();
+    assertThat(result.getPersistenceStatus().getMessage())
+        .isEqualTo("Cluster configuration service needs to be enabled");
+  }
+}

geode-assembly/src/integrationTest/java/org/apache/geode/management/internal/HttpServiceIntegrationTest.java

@@ -12,44 +12,73 @@
  * or implied. See the License for the specific language governing permissions and limitations under
  * the License.
  */
-package org.apache.geode.management.internal;
 
+package org.apache.geode.management.internal;
 
 import org.junit.ClassRule;
+import org.junit.Rule;
 import org.junit.Test;
 
 import org.apache.geode.test.junit.rules.GeodeDevRestClient;
 import org.apache.geode.test.junit.rules.RequiresGeodeHome;
 import org.apache.geode.test.junit.rules.ServerStarterRule;
 
-
 public class HttpServiceIntegrationTest {
-
   @ClassRule
   public static RequiresGeodeHome requiresGeodeHome = new RequiresGeodeHome();
 
-  @ClassRule
-  public static ServerStarterRule server =
-      new ServerStarterRule().withRestService().withJMXManager().withAutoStart();
+  @Rule
+  public ServerStarterRule server = new ServerStarterRule();
+
+  private GeodeDevRestClient client;
 
   @Test
-  public void devRestIsAvailable() throws Exception {
-    GeodeDevRestClient client =
+  public void withDevRestAndJmxManager() throws Exception {
+    server.withRestService().withJMXManager().startServer();
+    client =
         new GeodeDevRestClient("/geode/v1", "localhost", server.getHttpPort(), false);
     client.doGetAndAssert("/servers").hasStatusCode(200);
+
+    client =
+        new GeodeDevRestClient("/geode-mgmt/v1", "localhost", server.getHttpPort(), false);
+    client.doGetAndAssert("/version").hasStatusCode(200);
+
+    client =
+        new GeodeDevRestClient("/pulse", "localhost", server.getHttpPort(), false);
+    client.doGetAndAssert("/index.html").hasStatusCode(200);
+
   }
 
   @Test
-  public void adminRestIsAvailable() throws Exception {
-    GeodeDevRestClient client =
+  public void withDevRestOnly() throws Exception {
+    server.withRestService().startServer();
+    client =
+        new GeodeDevRestClient("/geode/v1", "localhost", server.getHttpPort(), false);
+    client.doGetAndAssert("/servers").hasStatusCode(200);
+
+    client =
         new GeodeDevRestClient("/geode-mgmt/v1", "localhost", server.getHttpPort(), false);
-    client.doGetAndAssert("/version").hasStatusCode(200);
+    client.doGetAndAssert("/version").hasStatusCode(404);
+
+    client =
+        new GeodeDevRestClient("/pulse", "localhost", server.getHttpPort(), false);
+    client.doGetAndAssert("/index.html").hasStatusCode(404);
   }
 
   @Test
-  public void pulseIsAvailable() throws Exception {
-    GeodeDevRestClient client =
+  public void withAdminRestOnly() throws Exception {
+    server.withJMXManager().withHttpService().startServer();
+    client =
+        new GeodeDevRestClient("/geode/v1", "localhost", server.getHttpPort(), false);
+    client.doGetAndAssert("/servers").hasStatusCode(404);
+
+    client =
+        new GeodeDevRestClient("/geode-mgmt/v1", "localhost", server.getHttpPort(), false);
+    client.doGetAndAssert("/version").hasStatusCode(200);
+
+    client =
         new GeodeDevRestClient("/pulse", "localhost", server.getHttpPort(), false);
     client.doGetAndAssert("/index.html").hasStatusCode(200);
   }
+
 }

geode-core/src/distributedTest/java/org/apache/geode/management/internal/api/RegionAPIDUnitTest.java

@@ -15,7 +15,6 @@
 package org.apache.geode.management.internal.api;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import org.junit.BeforeClass;
 import org.junit.ClassRule;
@@ -99,18 +98,6 @@ public void createsReplicatedRegion() {
     locator.invoke(() -> verifyRegionPersisted(regionName, "REPLICATE"));
   }
 
-  @Test
-  public void noName() {
-    locator.invoke(() -> {
-      RegionConfig config = new RegionConfig();
-      ClusterManagementService clusterManagementService =
-          ClusterStartupRule.getLocator().getClusterManagementService();
-      assertThatThrownBy(() -> clusterManagementService.create(config, "cluster"))
-          .isInstanceOf(IllegalArgumentException.class)
-          .hasMessageContaining("Name of the region has to be specified");
-    });
-  }
-
   @Test
   public void defaultTypeIsPartition() throws Exception {
     String regionName = testName.getMethodName();

geode-core/src/main/java/org/apache/geode/management/internal/api/LocatorClusterManagementService.java

@@ -43,7 +43,6 @@
 import org.apache.geode.management.internal.configuration.validators.ConfigurationValidator;
 import org.apache.geode.management.internal.configuration.validators.RegionConfigValidator;
 import org.apache.geode.management.internal.exceptions.EntityExistsException;
-import org.apache.geode.management.internal.exceptions.NoMembersException;
 
 public class LocatorClusterManagementService implements ClusterManagementService {
   private static Logger logger = LogService.getLogger();
@@ -77,27 +76,33 @@ public ClusterManagementResult create(CacheElement config, String group) {
       group = "cluster";
     }
 
+    if (persistenceService == null) {
+      return new ClusterManagementResult(false,
+          "Cluster configuration service needs to be enabled");
+    }
+
     ClusterManagementResult result = new ClusterManagementResult();
     ConfigurationMutator configurationMutator = mutators.get(config.getClass());
 
     ConfigurationValidator validator = validators.get(config.getClass());
     if (validator != null) {
-      validator.validate(config);
+      try {
+        validator.validate(config);
+      } catch (IllegalArgumentException e) {
+        return new ClusterManagementResult(false, e.getMessage());
+      }
     }
-    final boolean configurationPersistenceEnabled = persistenceService != null;
 
     // exit early if config element already exists in cache config
-    if (configurationPersistenceEnabled) {
-      CacheConfig currentPersistedConfig = persistenceService.getCacheConfig(group, true);
-      if (configurationMutator.exists(config, currentPersistedConfig)) {
-        throw new EntityExistsException("cache element " + config.getId() + " already exists.");
-      }
+    CacheConfig currentPersistedConfig = persistenceService.getCacheConfig(group, true);
+    if (configurationMutator.exists(config, currentPersistedConfig)) {
+      throw new EntityExistsException("cache element " + config.getId() + " already exists.");
     }
 
     // execute function on all members
     Set<DistributedMember> targetedMembers = findMembers(null, null);
     if (targetedMembers.size() == 0) {
-      throw new NoMembersException("no members found to create cache element");
+      return new ClusterManagementResult(false, "no members found to create cache element");
     }
 
     List<CliFunctionResult> functionResults = executeAndGetFunctionResult(
@@ -109,25 +114,26 @@ public ClusterManagementResult create(CacheElement config, String group) {
             functionResult.isSuccessful(),
             functionResult.getStatusMessage()));
 
-    // persist configuration in cache config
-    if (configurationPersistenceEnabled) {
-      String finalGroup = group;
-      persistenceService.updateCacheConfig(finalGroup, cacheConfigForGroup -> {
-        try {
-          configurationMutator.add(config, cacheConfigForGroup);
-          result.setClusterConfigPersisted(true,
-              "successfully persisted config for " + finalGroup);
-        } catch (Exception e) {
-          String message = "failed to update cluster config for " + finalGroup;
-          logger.error(message, e);
-          result.setClusterConfigPersisted(false, message);
-          return null;
-        }
-
-        return cacheConfigForGroup;
-      });
+    if (!result.isSuccessfullyAppliedOnMembers()) {
+      result.setClusterConfigPersisted(false, "Failed to apply the update on all members.");
+      return result;
     }
 
+    // persist configuration in cache config
+    String finalGroup = group;
+    persistenceService.updateCacheConfig(finalGroup, cacheConfigForGroup -> {
+      try {
+        configurationMutator.add(config, cacheConfigForGroup);
+        result.setClusterConfigPersisted(true,
+            "successfully persisted config for " + finalGroup);
+      } catch (Exception e) {
+        String message = "failed to update cluster config for " + finalGroup;
+        logger.error(message, e);
+        result.setClusterConfigPersisted(false, message);
+        return null;
+      }
+      return cacheConfigForGroup;
+    });
     return result;
   }
 
@@ -141,11 +147,13 @@ public ClusterManagementResult update(CacheElement config, String group) {
     throw new NotImplementedException();
   }
 
-  private Set<DistributedMember> findMembers(String[] groups, String[] members) {
+  @VisibleForTesting
+  Set<DistributedMember> findMembers(String[] groups, String[] members) {
     return CliUtil.findMembers(groups, members, distributionManager);
   }
 
-  private List<CliFunctionResult> executeAndGetFunctionResult(Function function, Object args,
+  @VisibleForTesting
+  List<CliFunctionResult> executeAndGetFunctionResult(Function function, Object args,
       Set<DistributedMember> targetMembers) {
     ResultCollector rc = CliUtil.executeFunction(function, args, targetMembers);
     return CliFunctionResult.cleanResults((List<?>) rc.getResult());

geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/CliFunctionResult.java

@@ -82,10 +82,6 @@ public CliFunctionResult(final String memberIdOrName, XmlEntity xmlEntity, final
     this.state = StatusState.OK;
   }
 
-  /**
-   * @deprecated Use {@code CliFunctionResult(String, StatusState, String)} instead
-   */
-  @Deprecated
   public CliFunctionResult(final String memberIdOrName, final boolean successful,
       final String message) {
     this(memberIdOrName, successful ? StatusState.OK : StatusState.ERROR, message);

geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/RegionConfigMutator.java

@@ -18,6 +18,7 @@
 package org.apache.geode.management.internal.configuration.mutators;
 
 import org.apache.geode.cache.configuration.CacheConfig;
+import org.apache.geode.cache.configuration.CacheElement;
 import org.apache.geode.cache.configuration.RegionConfig;
 
 public class RegionConfigMutator implements ConfigurationMutator<RegionConfig> {
@@ -31,7 +32,7 @@ public void add(RegionConfig configElement, CacheConfig existingConfig) {
 
   @Override
   public boolean exists(RegionConfig config, CacheConfig existing) {
-    return false;
+    return CacheElement.exists(existing.getRegions(), config.getId());
   }
 
   @Override