From cf2539997080194475ae94c95638cfa1d67a1689 Mon Sep 17 00:00:00 2001 From: "gihwan.kim" Date: Fri, 10 Dec 2021 15:25:48 +0900 Subject: [PATCH 1/8] Fix `updateStatus` doesn't handle replicating only --- .../internal/api/AdministrativeService.java | 22 ++++++ .../api/AdministrativeServiceTest.java | 70 ++++++++++++++++--- 2 files changed, 84 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java index 9450e32e6b..25eb977014 100644 --- a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java +++ b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java @@ -124,6 +124,28 @@ public CompletableFuture updateStatus(ServiceRequestContext ctx, } return status(); }); + } else if (oldStatus.replicating) { + if (!replicating) { // replicating -> unreplicating + logger.warn("Disabling replication with read-only mode"); + return executor().stop().handle((unused, cause) -> { + if (cause != null) { + logger.warn("Failed to stop the command executor:", cause); + } else { + logger.info("Disabled replication"); + } + return status(); + }); + } + } else if (replicating) { // unreplicating -> replicating + logger.warn("Enabling replication with read-only mode"); + return executor().start().handle((unused, cause) -> { + if (cause != null) { + logger.warn("Failed to start the command executor:", cause); + } else { + logger.info("Enabled replication"); + } + return status(); + }); } throw HttpStatusException.of(HttpStatus.NOT_MODIFIED); diff --git a/server/src/test/java/com/linecorp/centraldogma/server/internal/api/AdministrativeServiceTest.java b/server/src/test/java/com/linecorp/centraldogma/server/internal/api/AdministrativeServiceTest.java index b675174902..c7e011c480 100644 --- a/server/src/test/java/com/linecorp/centraldogma/server/internal/api/AdministrativeServiceTest.java +++ b/server/src/test/java/com/linecorp/centraldogma/server/internal/api/AdministrativeServiceTest.java @@ -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( @@ -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( @@ -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); } @@ -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); } @@ -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); } @@ -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 + " }"; + } } From d574f9b843f0568750eea46c79a64842af6aaa15 Mon Sep 17 00:00:00 2001 From: "gihwan.kim" Date: Wed, 15 Dec 2021 23:13:10 +0900 Subject: [PATCH 2/8] Simplify and add comment --- .../internal/api/AdministrativeService.java | 78 +++++++++---------- 1 file changed, 36 insertions(+), 42 deletions(-) diff --git a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java index 25eb977014..fc35842579 100644 --- a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java +++ b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java @@ -88,45 +88,49 @@ public CompletableFuture 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"); } - } else if (writable) { // unwritable -> writable - logger.warn("Leaving read-only mode .."); - executor().setWritable(true); - return executor().start().handle((unused, cause) -> { - if (cause != null) { - logger.warn("Failed to leave read-only mode:", cause); - } else { - logger.info("Left read-only mode"); - } - return status(); - }); - } else if (oldStatus.replicating) { - if (!replicating) { // replicating -> unreplicating - logger.warn("Disabling replication with read-only mode"); + } + + 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"); + } + return status(); + }); + } else { return executor().stop().handle((unused, cause) -> { if (cause != null) { logger.warn("Failed to stop the command executor:", cause); @@ -136,19 +140,9 @@ public CompletableFuture updateStatus(ServiceRequestContext ctx, return status(); }); } - } else if (replicating) { // unreplicating -> replicating - logger.warn("Enabling replication with read-only mode"); - return executor().start().handle((unused, cause) -> { - if (cause != null) { - logger.warn("Failed to start the command executor:", cause); - } else { - logger.info("Enabled replication"); - } - return status(); - }); } - throw HttpStatusException.of(HttpStatus.NOT_MODIFIED); + return CompletableFuture.completedFuture(status()); } private static CompletableFuture rejectStatusPatch(JsonNode patch) { From 704594579366644ca47b01bcf0d69fec4e7250a1 Mon Sep 17 00:00:00 2001 From: Gihwan Kim Date: Thu, 16 Dec 2021 17:30:56 +0900 Subject: [PATCH 3/8] Update server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java Co-authored-by: Ikhun Um --- .../centraldogma/server/internal/api/AdministrativeService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java index fc35842579..ea63c38d30 100644 --- a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java +++ b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java @@ -126,7 +126,7 @@ public CompletableFuture updateStatus(ServiceRequestContext ctx, if (cause != null) { logger.warn("Failed to start the command executor:", cause); } else { - logger.info("Enabled replication"); + logger.info("Enabled replication. read-only: {}", !writable); } return status(); }); From 1457610f1a184216631de959e39f8c390c0e487d Mon Sep 17 00:00:00 2001 From: Gihwan Kim Date: Thu, 16 Dec 2021 17:31:03 +0900 Subject: [PATCH 4/8] Update server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java Co-authored-by: Ikhun Um --- .../server/internal/api/AdministrativeService.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java index ea63c38d30..5a1396429b 100644 --- a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java +++ b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java @@ -114,9 +114,9 @@ public CompletableFuture updateStatus(ServiceRequestContext ctx, if (oldStatus.writable != writable) { executor().setWritable(writable); if (writable) { - logger.warn("Left read-only mode"); + logger.warn("Left read-only mode. replication: {}, replicating"); } else { - logger.warn("Entered read-only mode"); + logger.warn("Entered read-only mode. replication: {}, replicating"); } } From a3d3867ad9af93529a692ce4b5de95f4a21052f0 Mon Sep 17 00:00:00 2001 From: Gihwan Kim Date: Fri, 17 Dec 2021 17:59:10 +0900 Subject: [PATCH 5/8] Update server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java Co-authored-by: minux --- .../centraldogma/server/internal/api/AdministrativeService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java index 5a1396429b..bbda3328d2 100644 --- a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java +++ b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java @@ -114,7 +114,7 @@ public CompletableFuture updateStatus(ServiceRequestContext ctx, if (oldStatus.writable != writable) { executor().setWritable(writable); if (writable) { - logger.warn("Left read-only mode. replication: {}, replicating"); + logger.warn("Left read-only mode. replication: {}", replicating); } else { logger.warn("Entered read-only mode. replication: {}, replicating"); } From 43c2c7a13224bee1a76d2f56f2bcd70ca8cdd983 Mon Sep 17 00:00:00 2001 From: Gihwan Kim Date: Fri, 17 Dec 2021 17:59:18 +0900 Subject: [PATCH 6/8] Update server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java Co-authored-by: minux --- .../centraldogma/server/internal/api/AdministrativeService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java index bbda3328d2..959458d398 100644 --- a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java +++ b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java @@ -116,7 +116,7 @@ public CompletableFuture updateStatus(ServiceRequestContext ctx, if (writable) { logger.warn("Left read-only mode. replication: {}", replicating); } else { - logger.warn("Entered read-only mode. replication: {}, replicating"); + logger.warn("Entered read-only mode. replication: {}", replicating); } } From b2f31ead3a5bd46bcef9b616133bd890ad5c09c8 Mon Sep 17 00:00:00 2001 From: Gihwan Kim Date: Fri, 17 Dec 2021 18:00:16 +0900 Subject: [PATCH 7/8] Update server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java Co-authored-by: minux --- .../internal/api/AdministrativeService.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java index 959458d398..1e53c79476 100644 --- a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java +++ b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java @@ -130,15 +130,15 @@ public CompletableFuture updateStatus(ServiceRequestContext ctx, } return status(); }); - } else { - return executor().stop().handle((unused, cause) -> { - if (cause != null) { - logger.warn("Failed to stop the command executor:", cause); - } else { - logger.info("Disabled replication"); - } - return status(); - }); + } + return executor().stop().handle((unused, cause) -> { + if (cause != null) { + logger.warn("Failed to stop the command executor:", cause); + } else { + logger.info("Disabled replication"); + } + return status(); + }); } } From dc9f411ee0ab75b14acbe01d3e8a2addeebb28df Mon Sep 17 00:00:00 2001 From: "gihwan.kim" Date: Fri, 17 Dec 2021 18:17:58 +0900 Subject: [PATCH 8/8] Fix compile failure --- .../server/internal/api/AdministrativeService.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java index 1e53c79476..fafc680ff4 100644 --- a/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java +++ b/server/src/main/java/com/linecorp/centraldogma/server/internal/api/AdministrativeService.java @@ -114,7 +114,7 @@ public CompletableFuture updateStatus(ServiceRequestContext ctx, if (oldStatus.writable != writable) { executor().setWritable(writable); if (writable) { - logger.warn("Left read-only mode. replication: {}", replicating); + logger.warn("Left read-only mode."); } else { logger.warn("Entered read-only mode. replication: {}", replicating); } @@ -139,7 +139,6 @@ public CompletableFuture updateStatus(ServiceRequestContext ctx, } return status(); }); - } } return CompletableFuture.completedFuture(status());