You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by ji...@apache.org on 2019/02/21 19:17:04 UTC
[geode] branch develop updated: GEODE-6174: rest call "create"
should be idempotent (#3213)
This is an automated email from the ASF dual-hosted git repository.
jinmeiliao 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 73793a2 GEODE-6174: rest call "create" should be idempotent (#3213)
73793a2 is described below
commit 73793a2965bfa46ea4d7c4a5a685f0446cbf7814
Author: jinmeiliao <ji...@pivotal.io>
AuthorDate: Thu Feb 21 11:16:45 2019 -0800
GEODE-6174: rest call "create" should be idempotent (#3213)
Co-authored-by: Kenneth Howe <kh...@pivotal.io>
Co-authored-by: Peter Tran <pt...@pivotal.io>
---
.../internal/rest/RegionManagementDunitTest.java | 10 +++++----
.../internal/DisabledClusterConfigTest.java | 2 +-
.../api/LocatorClusterManagementService.java | 6 +++---
.../sanctioned-geode-core-serializables.txt | 1 -
.../internal/api/ClusterManagementResultTest.java | 19 +++++++++++------
.../api/LocatorClusterManagementServiceTest.java | 2 +-
.../internal/api/ClusterManagementResult.java | 24 +++++-----------------
.../geode/management/internal/api/Status.java | 21 ++++++-------------
.../rest/RegionManagementIntegrationTest.java | 2 +-
.../RegionManagementSecurityIntegrationTest.java | 8 ++++----
.../controllers/ManagementControllerAdvice.java | 6 +++---
11 files changed, 43 insertions(+), 58 deletions(-)
diff --git a/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/RegionManagementDunitTest.java b/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/RegionManagementDunitTest.java
index 628965d..679eaea 100644
--- a/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/RegionManagementDunitTest.java
+++ b/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/RegionManagementDunitTest.java
@@ -87,6 +87,7 @@ public class RegionManagementDunitTest {
assertThat(result.isSuccessfullyAppliedOnMembers()).isTrue();
assertThat(result.isSuccessfullyPersisted()).isTrue();
+ assertThat(result.isSuccessful()).isTrue();
assertThat(result.getMemberStatuses()).containsKeys("server-1").hasSize(1);
// make sure region is created
@@ -97,10 +98,11 @@ public class RegionManagementDunitTest {
// create the same region 2nd time
result = restClient.doPostAndAssert("/regions", json)
- .hasStatusCode(409)
+ .hasStatusCode(200)
.getClusterManagementResult();
- assertThat(result.isSuccessfullyAppliedOnMembers()).isFalse();
- assertThat(result.isSuccessfullyPersisted()).isFalse();
+ assertThat(result.isSuccessfullyAppliedOnMembers()).isTrue();
+ assertThat(result.isSuccessfullyPersisted()).isTrue();
+ assertThat(result.isSuccessful()).isTrue();
}
@Test
@@ -112,7 +114,7 @@ public class RegionManagementDunitTest {
.hasStatusCode(500)
.getClusterManagementResult();
- assertThat(result.isSuccessfullyAppliedOnMembers()).isFalse();
+ assertThat(result.isSuccessfullyAppliedOnMembers()).isTrue();
assertThat(result.isSuccessfullyPersisted()).isFalse();
}
diff --git a/geode-assembly/src/integrationTest/java/org/apache/geode/management/internal/DisabledClusterConfigTest.java b/geode-assembly/src/integrationTest/java/org/apache/geode/management/internal/DisabledClusterConfigTest.java
index 396c52f..f596830 100644
--- a/geode-assembly/src/integrationTest/java/org/apache/geode/management/internal/DisabledClusterConfigTest.java
+++ b/geode-assembly/src/integrationTest/java/org/apache/geode/management/internal/DisabledClusterConfigTest.java
@@ -49,7 +49,7 @@ public class DisabledClusterConfigTest {
.getClusterManagementResult();
assertThat(result.isSuccessful()).isFalse();
assertThat(result.isSuccessfullyPersisted()).isFalse();
- assertThat(result.isSuccessfullyAppliedOnMembers()).isFalse();
+ assertThat(result.isSuccessfullyAppliedOnMembers()).isTrue();
assertThat(result.getPersistenceStatus().getMessage())
.isEqualTo("Cluster configuration service needs to be enabled");
}
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 7cc41a6..74c35cb 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
@@ -115,7 +115,7 @@ public class LocatorClusterManagementService implements ClusterManagementService
functionResult.getStatusMessage()));
if (!result.isSuccessfullyAppliedOnMembers()) {
- result.setClusterConfigPersisted(false, "Failed to apply the update on all members.");
+ result.setPersistenceStatus(false, "Failed to apply the update on all members.");
return result;
}
@@ -124,12 +124,12 @@ public class LocatorClusterManagementService implements ClusterManagementService
persistenceService.updateCacheConfig(finalGroup, cacheConfigForGroup -> {
try {
configurationMutator.add(config, cacheConfigForGroup);
- result.setClusterConfigPersisted(true,
+ result.setPersistenceStatus(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);
+ result.setPersistenceStatus(false, message);
return null;
}
return cacheConfigForGroup;
diff --git a/geode-core/src/main/resources/org/apache/geode/internal/sanctioned-geode-core-serializables.txt b/geode-core/src/main/resources/org/apache/geode/internal/sanctioned-geode-core-serializables.txt
index 2ed429f..d4105c2 100644
--- a/geode-core/src/main/resources/org/apache/geode/internal/sanctioned-geode-core-serializables.txt
+++ b/geode-core/src/main/resources/org/apache/geode/internal/sanctioned-geode-core-serializables.txt
@@ -498,7 +498,6 @@ org/apache/geode/management/internal/JmxManagerLocator$StartJmxManagerFunction,t
org/apache/geode/management/internal/ManagementAgent$GemFireRMIServerSocketFactory,true,-811909050641332716,bindAddr:java/net/InetAddress
org/apache/geode/management/internal/ManagementFunction,true,1,mbeanServer:javax/management/MBeanServer,notificationHub:org/apache/geode/management/internal/NotificationHub
org/apache/geode/management/internal/NotificationKey,false,currentTime:long,objectName:javax/management/ObjectName
-org/apache/geode/management/internal/api/Status$Result,false0
org/apache/geode/management/internal/beans/FileUploader$RemoteFile,false,filename:java/lang/String,outputStream:com/healthmarketscience/rmiio/RemoteOutputStream
org/apache/geode/management/internal/beans/QueryDataFunction,true,1
org/apache/geode/management/internal/beans/QueryDataFunction$LocalQueryFunction,true,1,id:java/lang/String,optimizeForWrite:boolean,regionName:java/lang/String,showMembers:boolean,this$0:org/apache/geode/management/internal/beans/QueryDataFunction
diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/api/ClusterManagementResultTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/api/ClusterManagementResultTest.java
index 57acc1d..d58c919 100644
--- a/geode-core/src/test/java/org/apache/geode/management/internal/api/ClusterManagementResultTest.java
+++ b/geode-core/src/test/java/org/apache/geode/management/internal/api/ClusterManagementResultTest.java
@@ -34,6 +34,7 @@ public class ClusterManagementResultTest {
public void failsWhenNotAppliedOnAllMembers() {
result.addMemberStatus("member-1", true, "msg-1");
result.addMemberStatus("member-2", false, "msg-2");
+ result.setPersistenceStatus(true, "message");
assertThat(result.isSuccessfullyAppliedOnMembers()).isFalse();
assertThat(result.isSuccessful()).isFalse();
}
@@ -42,13 +43,14 @@ public class ClusterManagementResultTest {
public void successfulOnlyWhenResultIsSuccessfulOnAllMembers() {
result.addMemberStatus("member-1", true, "msg-1");
result.addMemberStatus("member-2", true, "msg-2");
+ result.setPersistenceStatus(true, "message");
assertThat(result.isSuccessfullyAppliedOnMembers()).isTrue();
assertThat(result.isSuccessful()).isTrue();
}
@Test
public void emptyMemberStatus() {
- assertThat(result.isSuccessfullyAppliedOnMembers()).isFalse();
+ assertThat(result.isSuccessfullyAppliedOnMembers()).isTrue();
assertThat(result.isSuccessfullyPersisted()).isFalse();
assertThat(result.isSuccessful()).isFalse();
}
@@ -56,16 +58,21 @@ public class ClusterManagementResultTest {
@Test
public void failsWhenNotPersisted() {
- result.setClusterConfigPersisted(false, "msg-1");
+ result.setPersistenceStatus(false, "msg-1");
assertThat(result.isSuccessfullyPersisted()).isFalse();
assertThat(result.isSuccessful()).isFalse();
}
@Test
- public void failsWhenNoMembersExists() {
- result.setClusterConfigPersisted(true, "msg-1");
- assertThat(result.isSuccessfullyPersisted()).isTrue();
- assertThat(result.isSuccessfullyAppliedOnMembers()).isFalse();
+ public void whenNoMembersExists() {
+ result.setPersistenceStatus(false, "msg-1");
+ assertThat(result.isSuccessfullyPersisted()).isFalse();
+ assertThat(result.isSuccessfullyAppliedOnMembers()).isTrue();
assertThat(result.isSuccessful()).isFalse();
+
+ result.setPersistenceStatus(true, "msg-1");
+ assertThat(result.isSuccessfullyPersisted()).isTrue();
+ assertThat(result.isSuccessfullyAppliedOnMembers()).isTrue();
+ assertThat(result.isSuccessful()).isTrue();
}
}
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 ad1031f..f6aa96d 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
@@ -90,7 +90,7 @@ public class LocatorClusterManagementServiceTest {
doReturn(Collections.emptySet()).when(service).findMembers(any(), any());
result = service.create(regionConfig, "cluster");
assertThat(result.isSuccessful()).isFalse();
- assertThat(result.isSuccessfullyAppliedOnMembers()).isFalse();
+ assertThat(result.isSuccessfullyAppliedOnMembers()).isTrue();
assertThat(result.getPersistenceStatus().getMessage())
.contains("no members found to create cache element");
}
diff --git a/geode-management/src/main/java/org/apache/geode/management/internal/api/ClusterManagementResult.java b/geode-management/src/main/java/org/apache/geode/management/internal/api/ClusterManagementResult.java
index 829dfb3..a4b03fb 100644
--- a/geode-management/src/main/java/org/apache/geode/management/internal/api/ClusterManagementResult.java
+++ b/geode-management/src/main/java/org/apache/geode/management/internal/api/ClusterManagementResult.java
@@ -23,7 +23,7 @@ import com.fasterxml.jackson.annotation.JsonIgnore;
public class ClusterManagementResult {
private Map<String, Status> memberStatuses = new HashMap<>();
- private Status persistenceStatus = new Status(Status.Result.NOT_APPLICABLE, null);
+ private Status persistenceStatus = new Status();
public ClusterManagementResult() {}
@@ -31,15 +31,11 @@ public class ClusterManagementResult {
this.persistenceStatus = new Status(success, message);
}
- public void addMemberStatus(String member, Status.Result result, String message) {
- this.memberStatuses.put(member, new Status(result, message));
- }
-
public void addMemberStatus(String member, boolean success, String message) {
this.memberStatuses.put(member, new Status(success, message));
}
- public void setClusterConfigPersisted(boolean success, String message) {
+ public void setPersistenceStatus(boolean success, String message) {
this.persistenceStatus = new Status(success, message);
}
@@ -53,27 +49,17 @@ public class ClusterManagementResult {
@JsonIgnore
public boolean isSuccessfullyAppliedOnMembers() {
- if (memberStatuses.isEmpty()) {
- return false;
- }
- return memberStatuses.values().stream().allMatch(x -> x.status == Status.Result.SUCCESS);
+ return memberStatuses.values().stream().allMatch(x -> x.success);
}
@JsonIgnore
public boolean isSuccessfullyPersisted() {
- return persistenceStatus.status == Status.Result.SUCCESS;
+ return persistenceStatus.isSuccess();
}
- /**
- * - true if operation is successful on all distributed members,
- * and configuration persistence is either not applicable (in case cluster config is disabled)
- * or configuration persistence is applicable and successful
- * - false otherwise
- */
@JsonIgnore
public boolean isSuccessful() {
- return (persistenceStatus.status == Status.Result.NOT_APPLICABLE || isSuccessfullyPersisted())
- && isSuccessfullyAppliedOnMembers();
+ return isSuccessfullyPersisted() && isSuccessfullyAppliedOnMembers();
}
}
diff --git a/geode-management/src/main/java/org/apache/geode/management/internal/api/Status.java b/geode-management/src/main/java/org/apache/geode/management/internal/api/Status.java
index 097e4ce..9a8a5cb 100644
--- a/geode-management/src/main/java/org/apache/geode/management/internal/api/Status.java
+++ b/geode-management/src/main/java/org/apache/geode/management/internal/api/Status.java
@@ -15,32 +15,23 @@
package org.apache.geode.management.internal.api;
public class Status {
- enum Result {
- SUCCESS, FAILURE, NOT_APPLICABLE
- }
-
- Result status;
+ boolean success;
String message;
// needed for json deserialization
public Status() {}
- public Status(Result status, String message) {
- this.status = status;
- this.message = message;
- }
-
public Status(boolean success, String message) {
- this.status = success ? Result.SUCCESS : Result.FAILURE;
+ this.success = success;
this.message = message;
}
- public Result getStatus() {
- return status;
+ public boolean isSuccess() {
+ return success;
}
- public void setStatus(Result status) {
- this.status = status;
+ public void setSuccess(boolean success) {
+ this.success = success;
}
public String getMessage() {
diff --git a/geode-web-management/src/integrationTest/java/org/apache/geode/management/internal/rest/RegionManagementIntegrationTest.java b/geode-web-management/src/integrationTest/java/org/apache/geode/management/internal/rest/RegionManagementIntegrationTest.java
index afca659..5df6a11 100644
--- a/geode-web-management/src/integrationTest/java/org/apache/geode/management/internal/rest/RegionManagementIntegrationTest.java
+++ b/geode-web-management/src/integrationTest/java/org/apache/geode/management/internal/rest/RegionManagementIntegrationTest.java
@@ -72,7 +72,7 @@ public class RegionManagementIntegrationTest {
.with(POST_PROCESSOR)
.content(json))
.andExpect(status().isInternalServerError())
- .andExpect(jsonPath("$.persistenceStatus.status", is("FAILURE")))
+ .andExpect(jsonPath("$.persistenceStatus.success", is(false)))
.andExpect(jsonPath("$.persistenceStatus.message",
is("no members found to create cache element")));
}
diff --git a/geode-web-management/src/integrationTest/java/org/apache/geode/management/internal/rest/RegionManagementSecurityIntegrationTest.java b/geode-web-management/src/integrationTest/java/org/apache/geode/management/internal/rest/RegionManagementSecurityIntegrationTest.java
index 5516254..0acce32 100644
--- a/geode-web-management/src/integrationTest/java/org/apache/geode/management/internal/rest/RegionManagementSecurityIntegrationTest.java
+++ b/geode-web-management/src/integrationTest/java/org/apache/geode/management/internal/rest/RegionManagementSecurityIntegrationTest.java
@@ -73,7 +73,7 @@ public class RegionManagementSecurityIntegrationTest {
.with(POST_PROCESSOR)
.content(json))
.andExpect(status().isForbidden())
- .andExpect(jsonPath("$.persistenceStatus.status", is("FAILURE")))
+ .andExpect(jsonPath("$.persistenceStatus.success", is(false)))
.andExpect(jsonPath("$.persistenceStatus.message",
is("user not authorized for DATA:MANAGE")));
}
@@ -84,7 +84,7 @@ public class RegionManagementSecurityIntegrationTest {
.with(POST_PROCESSOR)
.content(json))
.andExpect(status().isUnauthorized())
- .andExpect(jsonPath("$.persistenceStatus.status", is("FAILURE")))
+ .andExpect(jsonPath("$.persistenceStatus.success", is(false)))
.andExpect(jsonPath("$.persistenceStatus.message",
is("Full authentication is required to access this resource")));
}
@@ -96,7 +96,7 @@ public class RegionManagementSecurityIntegrationTest {
.with(POST_PROCESSOR)
.content(json))
.andExpect(status().isUnauthorized())
- .andExpect(jsonPath("$.persistenceStatus.status", is("FAILURE")))
+ .andExpect(jsonPath("$.persistenceStatus.success", is(false)))
.andExpect(jsonPath("$.persistenceStatus.message",
is("Authentication error. Please check your credentials.")));
}
@@ -108,7 +108,7 @@ public class RegionManagementSecurityIntegrationTest {
.with(POST_PROCESSOR)
.content(json))
.andExpect(status().isInternalServerError())
- .andExpect(jsonPath("$.persistenceStatus.status", is("FAILURE")))
+ .andExpect(jsonPath("$.persistenceStatus.success", is(false)))
.andExpect(jsonPath("$.persistenceStatus.message",
is("no members found to create cache element")));
}
diff --git a/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/ManagementControllerAdvice.java b/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/ManagementControllerAdvice.java
index 43358ff..dba5499 100644
--- a/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/ManagementControllerAdvice.java
+++ b/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/controllers/ManagementControllerAdvice.java
@@ -44,9 +44,9 @@ public class ManagementControllerAdvice {
@ExceptionHandler(EntityExistsException.class)
public ResponseEntity<ClusterManagementResult> entityExists(final Exception e) {
- // no need to log the error stack. User only needs to know the message.
- return new ResponseEntity<>(new ClusterManagementResult(false, e.getMessage()),
- HttpStatus.CONFLICT);
+ // for idempotency, we treat EntityExistsException as OK
+ return new ResponseEntity<>(new ClusterManagementResult(true, e.getMessage()),
+ HttpStatus.OK);
}
@ExceptionHandler({AuthenticationFailedException.class, AuthenticationException.class})