Skip to content

Commit

Permalink
Fix updateStatus doesn't update replication when read-only mode (#656)
Browse files Browse the repository at this point in the history 
### Motivation

 - #360

### Modification

 - Enable replication when read-only with not-replication mode and given `replicating` is `true`.
 - Disable replication when read-only with replication mode and given `replicating` is `false`.

### Result

 - Fix #360 (?)
 - Users can enable/disable replication when read-only mode.
  • Loading branch information
Gihwan Kim authored Dec 23, 2021
1 parent e46e977 commit d596a34
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 32 deletions.
63 changes: 39 additions & 24 deletions ...er/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,45 +88,60 @@ public CompletableFuture<ServerStatus> updateStatus(ServiceRequestContext ctx,
return rejectStatusPatch(patch);
}

// writable | replicating | result
//-------------------+---------------+-----------------------------------
// true -> true | true -> true | not_modified exception
// | true -> false | bad_request exception
//-------------------+---------------+-----------------------------------
// true -> false | true -> true | setWritable(false)
// | true -> false | setWritable(false) & stop executor.
//-------------------+---------------+-----------------------------------
// false -> true | true -> true | setWritable(true)
// | false -> true | setWritable(true) & start executor.
//-------------------+---------------+-----------------------------------
// false -> false | true -> false | Stop executor.
// | false -> true | Start executor.
final boolean writable = writableNode.asBoolean();
final boolean replicating = replicatingNode.asBoolean();
if (writable && !replicating) {
return HttpApiUtil.throwResponse(ctx, HttpStatus.BAD_REQUEST,
"'replicating' must be 'true' if 'writable' is 'true'.");
}
if (oldStatus.writable == writable && oldStatus.replicating == replicating) {
throw HttpStatusException.of(HttpStatus.NOT_MODIFIED);
}

if (oldStatus.writable) {
if (!writable) { // writable -> unwritable
executor().setWritable(false);
if (replicating) {
logger.warn("Entered read-only mode with replication enabled");
return CompletableFuture.completedFuture(status());
} else {
logger.warn("Entering read-only mode with replication disabled ..");
return executor().stop().handle((unused, cause) -> {
if (cause != null) {
logger.warn("Failed to stop the command executor:", cause);
} else {
logger.info("Entered read-only mode with replication disabled");
}
return status();
});
}
if (oldStatus.writable != writable) {
executor().setWritable(writable);
if (writable) {
logger.warn("Left read-only mode.");
} else {
logger.warn("Entered read-only mode. replication: {}", replicating);
}
}

if (oldStatus.replicating != replicating) {
if (replicating) {
return executor().start().handle((unused, cause) -> {
if (cause != null) {
logger.warn("Failed to start the command executor:", cause);
} else {
logger.info("Enabled replication. read-only: {}", !writable);
}
return status();
});
}
} else if (writable) { // unwritable -> writable
logger.warn("Leaving read-only mode ..");
executor().setWritable(true);
return executor().start().handle((unused, cause) -> {
return executor().stop().handle((unused, cause) -> {
if (cause != null) {
logger.warn("Failed to leave read-only mode:", cause);
logger.warn("Failed to stop the command executor:", cause);
} else {
logger.info("Left read-only mode");
logger.info("Disabled replication");
}
return status();
});
}

throw HttpStatusException.of(HttpStatus.NOT_MODIFIED);
return CompletableFuture.completedFuture(status());
}

private static CompletableFuture<ServerStatus> rejectStatusPatch(JsonNode patch) {
Expand Down
70 changes: 62 additions & 8 deletions ...rc/test/java/com/linecorp/centraldogma/server/internal/api/AdministrativeServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ void updateStatus_setUnwritable() {
final AggregatedHttpResponse res = client.execute(
RequestHeaders.of(HttpMethod.PATCH, API_V1_PATH_PREFIX + "status",
HttpHeaderNames.CONTENT_TYPE, MediaType.JSON_PATCH),
"[{ \"op\": \"replace\", \"path\": \"/writable\", \"value\": false }]").aggregate().join();
"[" + writable(false) + "]").aggregate().join();

assertThat(res.status()).isEqualTo(HttpStatus.OK);
assertThatJson(res.contentUtf8()).isEqualTo(
Expand All @@ -76,8 +76,7 @@ void updateStatus_setUnwritableAndNonReplicating() {
final AggregatedHttpResponse res = client.execute(
RequestHeaders.of(HttpMethod.PATCH, API_V1_PATH_PREFIX + "status",
HttpHeaderNames.CONTENT_TYPE, MediaType.JSON_PATCH),
"[{ \"op\": \"replace\", \"path\": \"/writable\", \"value\": false }," +
" { \"op\": \"replace\", \"path\": \"/replicating\", \"value\": false }]").aggregate().join();
"[" + writable(false) + "," + replicating(false) + "]").aggregate().join();

assertThat(res.status()).isEqualTo(HttpStatus.OK);
assertThatJson(res.contentUtf8()).isEqualTo(
Expand All @@ -90,8 +89,7 @@ void updateStatus_setWritableAndNonReplicating() {
final AggregatedHttpResponse res = client.execute(
RequestHeaders.of(HttpMethod.PATCH, API_V1_PATH_PREFIX + "status",
HttpHeaderNames.CONTENT_TYPE, MediaType.JSON_PATCH),
"[{ \"op\": \"replace\", \"path\": \"/writable\", \"value\": true }," +
" { \"op\": \"replace\", \"path\": \"/replicating\", \"value\": false }]").aggregate().join();
"[" + writable(true) + "," + replicating(false) + "]").aggregate().join();

assertThat(res.status()).isEqualTo(HttpStatus.BAD_REQUEST);
}
Expand All @@ -102,7 +100,7 @@ void redundantUpdateStatus_Writable() {
final AggregatedHttpResponse res = client.execute(
RequestHeaders.of(HttpMethod.PATCH, API_V1_PATH_PREFIX + "status",
HttpHeaderNames.CONTENT_TYPE, MediaType.JSON_PATCH),
"[{ \"op\": \"replace\", \"path\": \"/writable\", \"value\": true }]").aggregate().join();
"[" + writable(true) + "]").aggregate().join();

assertThat(res.status()).isEqualTo(HttpStatus.NOT_MODIFIED);
}
Expand All @@ -113,7 +111,7 @@ void redundantUpdateStatus_Replicating() {
final AggregatedHttpResponse res = client.execute(
RequestHeaders.of(HttpMethod.PATCH, API_V1_PATH_PREFIX + "status",
HttpHeaderNames.CONTENT_TYPE, MediaType.JSON_PATCH),
"[{ \"op\": \"replace\", \"path\": \"/replicating\", \"value\": true }]").aggregate().join();
"[" + replicating(true) + "]").aggregate().join();

assertThat(res.status()).isEqualTo(HttpStatus.NOT_MODIFIED);
}
Expand All @@ -127,10 +125,66 @@ void updateStatus_leaveReadOnlyMode() {
final AggregatedHttpResponse res = client.execute(
RequestHeaders.of(HttpMethod.PATCH, API_V1_PATH_PREFIX + "status",
HttpHeaderNames.CONTENT_TYPE, MediaType.JSON_PATCH),
"[{ \"op\": \"replace\", \"path\": \"/writable\", \"value\": true }]").aggregate().join();
"[" + writable(true) + "]").aggregate().join();

assertThat(res.status()).isEqualTo(HttpStatus.OK);
assertThatJson(res.contentUtf8()).isEqualTo(
"{ \"writable\": true, \"replicating\": true }");
}

@Test
void updateStatus_enableReplicatingWithReadOnlyMode() {
final WebClient client = dogma.httpClient();

// Try to enter read-only mode with replication disabled.
AggregatedHttpResponse res =
client.execute(RequestHeaders.of(HttpMethod.PATCH, API_V1_PATH_PREFIX + "status",
HttpHeaderNames.CONTENT_TYPE, MediaType.JSON_PATCH),
"[" + writable(false) + "," + replicating(false) + "]")
.aggregate()
.join();
assertThat(res.status()).isEqualTo(HttpStatus.OK);
assertThatJson(res.contentUtf8()).isEqualTo("{ \"writable\": false, \"replicating\": false }");

// Try to enable replication.
res = client.execute(RequestHeaders.of(HttpMethod.PATCH, API_V1_PATH_PREFIX + "status",
HttpHeaderNames.CONTENT_TYPE, MediaType.JSON_PATCH),
"[" + replicating(true) + "]")
.aggregate()
.join();
assertThat(res.status()).isEqualTo(HttpStatus.OK);
assertThatJson(res.contentUtf8()).isEqualTo("{ \"writable\": false, \"replicating\": true }");
}

@Test
void updateStatus_disableReplicatingWithReadOnlyMode() {
final WebClient client = dogma.httpClient();

// Try to enter read-only mode with replication enabled.
AggregatedHttpResponse res =
client.execute(RequestHeaders.of(HttpMethod.PATCH, API_V1_PATH_PREFIX + "status",
HttpHeaderNames.CONTENT_TYPE, MediaType.JSON_PATCH),
"[" + writable(false) + "," + replicating(true) + "]")
.aggregate()
.join();
assertThat(res.status()).isEqualTo(HttpStatus.OK);
assertThatJson(res.contentUtf8()).isEqualTo("{ \"writable\": false, \"replicating\": true }");

// Try to disable replication.
res = client.execute(RequestHeaders.of(HttpMethod.PATCH, API_V1_PATH_PREFIX + "status",
HttpHeaderNames.CONTENT_TYPE, MediaType.JSON_PATCH),
"[" + replicating(false) + "]")
.aggregate()
.join();
assertThat(res.status()).isEqualTo(HttpStatus.OK);
assertThatJson(res.contentUtf8()).isEqualTo("{ \"writable\": false, \"replicating\": false }");
}

private static String writable(boolean writable) {
return "{ \"op\": \"replace\", \"path\": \"/writable\", \"value\": " + writable + " }";
}

private static String replicating(boolean replicating) {
return "{ \"op\": \"replace\", \"path\": \"/replicating\", \"value\": " + replicating + " }";
}
}

0 comments on commit d596a34

Please sign in to comment.