From 6b1d46746e2998045e9ef7194c47af7231068bea Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Tue, 5 Sep 2017 09:03:29 +0200 Subject: Revert "Revert "Return 409 if there is a conflict when activating"" --- .../config/server/ActivationConflictException.java | 15 ++++++++++++++ .../vespa/config/server/deploy/Deployment.java | 13 ++++++------ .../config/server/http/HttpErrorResponse.java | 5 +++++ .../vespa/config/server/http/HttpHandler.java | 3 +++ .../server/http/v2/SessionActiveHandlerTest.java | 23 +++++++++++++--------- 5 files changed, 44 insertions(+), 15 deletions(-) create mode 100644 configserver/src/main/java/com/yahoo/vespa/config/server/ActivationConflictException.java (limited to 'configserver') diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/ActivationConflictException.java b/configserver/src/main/java/com/yahoo/vespa/config/server/ActivationConflictException.java new file mode 100644 index 00000000000..48023293cc4 --- /dev/null +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/ActivationConflictException.java @@ -0,0 +1,15 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.config.server; + +/** + * Exception used when activation cannot be done because activation is for + * an older session than the one that is active now or because current active + * session has changed since the session to be activated was created + * + * @author hmusum + */ +public class ActivationConflictException extends RuntimeException { + public ActivationConflictException(String s) { + super(s); + } +} diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java index 51995eb98cf..14c130ab463 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java @@ -9,6 +9,7 @@ import com.yahoo.log.LogLevel; import com.yahoo.path.Path; import com.yahoo.transaction.NestedTransaction; import com.yahoo.transaction.Transaction; +import com.yahoo.vespa.config.server.ActivationConflictException; import com.yahoo.vespa.config.server.tenant.ActivateLock; import com.yahoo.vespa.config.server.ApplicationRepository; import com.yahoo.vespa.config.server.TimeoutBudget; @@ -202,25 +203,25 @@ public class Deployment implements com.yahoo.config.provision.Deployment { ", current active session=" + currentActiveSessionSessionId); if (currentActiveSession.isNewerThan(activeSessionAtCreate) && currentActiveSessionSessionId != sessionId) { - String errMsg = currentActiveSession.logPre()+"Cannot activate session " + + String errMsg = currentActiveSession.logPre() + "Cannot activate session " + sessionId + " because the currently active session (" + currentActiveSessionSessionId + ") has changed since session " + sessionId + " was created (was " + activeSessionAtCreate + " at creation time)"; if (ignoreStaleSessionFailure) { log.warning(errMsg + " (Continuing because of force.)"); } else { - throw new IllegalStateException(errMsg); + throw new ActivationConflictException(errMsg); } } } - // As of now, config generation is based on session id, and config generation must be an monotonically + // As of now, config generation is based on session id, and config generation must be a monotonically // increasing number private void checkIfActiveIsNewerThanSessionToBeActivated(long sessionId, long currentActiveSessionId) { if (sessionId < currentActiveSessionId) { - throw new IllegalArgumentException("It is not possible to activate session " + sessionId + - ", because it is older than current active session (" + - currentActiveSessionId + ")"); + throw new ActivationConflictException("It is not possible to activate session " + sessionId + + ", because it is older than current active session (" + + currentActiveSessionId + ")"); } } diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpErrorResponse.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpErrorResponse.java index 7e0c57f2e5f..7a86357cd95 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpErrorResponse.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpErrorResponse.java @@ -35,6 +35,7 @@ public class HttpErrorResponse extends HttpResponse { public enum errorCodes { APPLICATION_LOCK_FAILURE, BAD_REQUEST, + ACTIVATION_FAILED, INTERNAL_SERVER_ERROR, INVALID_APPLICATION_PACKAGE, METHOD_NOT_ALLOWED, @@ -64,6 +65,10 @@ public class HttpErrorResponse extends HttpResponse { return new HttpErrorResponse(BAD_REQUEST, errorCodes.BAD_REQUEST.name(), msg); } + public static HttpErrorResponse conflictWhenActivating(String msg) { + return new HttpErrorResponse(CONFLICT, errorCodes.ACTIVATION_FAILED.name(), msg); + } + public static HttpErrorResponse methodNotAllowed(String msg) { return new HttpErrorResponse(METHOD_NOT_ALLOWED, errorCodes.METHOD_NOT_ALLOWED.name(), msg); } diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpHandler.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpHandler.java index 53bfbf5135f..c13d1b3fcfa 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpHandler.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/HttpHandler.java @@ -8,6 +8,7 @@ import com.yahoo.container.jdisc.LoggingRequestHandler; import com.yahoo.container.logging.AccessLog; import com.yahoo.log.LogLevel; import com.yahoo.config.provision.OutOfCapacityException; +import com.yahoo.vespa.config.server.ActivationConflictException; import com.yahoo.yolean.Exceptions; import java.io.PrintWriter; @@ -47,6 +48,8 @@ public class HttpHandler extends LoggingRequestHandler { } } catch (NotFoundException | com.yahoo.vespa.config.server.NotFoundException e) { return HttpErrorResponse.notFoundError(getMessage(e, request)); + } catch (ActivationConflictException e) { + return HttpErrorResponse.conflictWhenActivating(getMessage(e, request)); } catch (BadRequestException | IllegalArgumentException | IllegalStateException e) { return HttpErrorResponse.badRequest(getMessage(e, request)); } catch (OutOfCapacityException e) { diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionActiveHandlerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionActiveHandlerTest.java index 7fca78b087b..f6108611f73 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionActiveHandlerTest.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionActiveHandlerTest.java @@ -67,8 +67,12 @@ import java.time.Clock; import java.util.Collections; import java.util.Optional; -import static com.yahoo.jdisc.Response.Status.*; +import static com.yahoo.jdisc.Response.Status.BAD_REQUEST; +import static com.yahoo.jdisc.Response.Status.CONFLICT; +import static com.yahoo.jdisc.Response.Status.INTERNAL_SERVER_ERROR; import static com.yahoo.jdisc.Response.Status.METHOD_NOT_ALLOWED; +import static com.yahoo.jdisc.Response.Status.NOT_FOUND; +import static com.yahoo.jdisc.Response.Status.OK; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; @@ -154,7 +158,7 @@ public class SessionActiveHandlerTest extends SessionHandlerTest { ActivateRequest activateRequest = new ActivateRequest(sessionId, 1, "", Clock.systemUTC()).invoke(); HttpResponse actResponse = activateRequest.getActResponse(); String message = getRenderedString(actResponse); - assertThat(message, actResponse.getStatus(), Is.is(BAD_REQUEST)); + assertThat(message, actResponse.getStatus(), Is.is(CONFLICT)); assertThat(message, containsString("Cannot activate session 3 because the currently active session (2) has changed since session 3 was created (was 1 at creation time)")); } @@ -178,8 +182,8 @@ public class SessionActiveHandlerTest extends SessionHandlerTest { Clock clock = Clock.systemUTC(); activateAndAssertOK(90l, 0l, clock); activateAndAssertError(92l, 89l, clock, - HttpErrorResponse.errorCodes.BAD_REQUEST, - "tenant:"+tenant+" app:default:default Cannot activate session 92 because the currently active session (90) has changed since session 92 was created (was 89 at creation time)"); + Response.Status.CONFLICT, HttpErrorResponse.errorCodes.ACTIVATION_FAILED, + "tenant:" + tenant + " app:default:default Cannot activate session 92 because the currently active session (90) has changed since session 92 was created (was 89 at creation time)"); } @Test @@ -209,7 +213,7 @@ public class SessionActiveHandlerTest extends SessionHandlerTest { public void require_that_handler_gives_error_when_provisioner_activated_fails() throws Exception { hostProvisioner = new FailingMockProvisioner(); hostProvisioner.activated = false; - activateAndAssertError(1, 0, Clock.systemUTC(), HttpErrorResponse.errorCodes.BAD_REQUEST, "Cannot activate application"); + activateAndAssertError(1, 0, Clock.systemUTC(), BAD_REQUEST, HttpErrorResponse.errorCodes.BAD_REQUEST, "Cannot activate application"); assertFalse(hostProvisioner.activated); } @@ -249,12 +253,13 @@ public class SessionActiveHandlerTest extends SessionHandlerTest { return activateRequest; } - private ActivateRequest activateAndAssertErrorPut(long sessionId, long previousSessionId, Clock clock, HttpErrorResponse.errorCodes errorCode, String expectedError) throws Exception { + private ActivateRequest activateAndAssertErrorPut(long sessionId, long previousSessionId, Clock clock, + int statusCode, HttpErrorResponse.errorCodes errorCode, String expectedError) throws Exception { ActivateRequest activateRequest = new ActivateRequest(sessionId, previousSessionId, "", clock); activateRequest.invoke(); HttpResponse actResponse = activateRequest.getActResponse(); RemoteSession session = activateRequest.getSession(); - assertThat(actResponse.getStatus(), Is.is(BAD_REQUEST)); + assertThat(actResponse.getStatus(), Is.is(statusCode)); String message = getRenderedString(actResponse); assertThat(message, Is.is("{\"error-code\":\"" + errorCode.name() + "\",\"message\":\"" + expectedError + "\"}")); assertThat(session.getStatus(), Is.is(Session.Status.PREPARE)); @@ -353,9 +358,9 @@ public class SessionActiveHandlerTest extends SessionHandlerTest { assertThat(hostProvisioner.lastHosts.size(), is(1)); } - private void activateAndAssertError(long sessionId, long previousSessionId, Clock clock, HttpErrorResponse.errorCodes errorCode, String expectedError) throws Exception { + private void activateAndAssertError(long sessionId, long previousSessionId, Clock clock, int statusCode, HttpErrorResponse.errorCodes errorCode, String expectedError) throws Exception { hostProvisioner.activated = false; - activateAndAssertErrorPut(sessionId, previousSessionId, clock, errorCode, expectedError); + activateAndAssertErrorPut(sessionId, previousSessionId, clock, statusCode, errorCode, expectedError); assertFalse(hostProvisioner.activated); } -- cgit v1.2.3