Michael Blow has submitted this change and it was merged. Change subject: [NO ISSUE] Cleanup query service exception handling ......................................................................
[NO ISSUE] Cleanup query service exception handling Change-Id: I1f2828481df055d6c96f1ae1869ef37a065bf576 Reviewed-on: https://asterix-gerrit.ics.uci.edu/2524 Sonar-Qube: Jenkins <[email protected]> Tested-by: Jenkins <[email protected]> Contrib: Jenkins <[email protected]> Integration-Tests: Jenkins <[email protected]> Reviewed-by: Murtadha Hubail <[email protected]> --- M asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCQueryServiceServlet.java M asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java M asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/ExecuteStatementRequestMessage.java M asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/ErrorCode.java M asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties M hyracks-fullstack/algebricks/algebricks-common/src/main/java/org/apache/hyracks/algebricks/common/exceptions/AlgebricksException.java M hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/HyracksException.java 7 files changed, 54 insertions(+), 28 deletions(-) Approvals: Anon. E. Moose #1000171: Jenkins: Verified; No violations found; ; Verified Murtadha Hubail: Looks good to me, approved diff --git a/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCQueryServiceServlet.java b/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCQueryServiceServlet.java index 3564736..a420efc 100644 --- a/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCQueryServiceServlet.java +++ b/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCQueryServiceServlet.java @@ -154,13 +154,13 @@ } @Override - protected void handleExecuteStatementException(Throwable t, RequestExecutionState execution) { - if (t instanceof TimeoutException + protected void handleExecuteStatementException(Throwable t, RequestExecutionState state, RequestParameters param) { + if (t instanceof TimeoutException // TODO(mblow): I don't think t can ever been an instance of TimeoutException || ExceptionUtils.matchingCause(t, candidate -> candidate instanceof IPCException)) { GlobalConfig.ASTERIX_LOGGER.log(Level.WARN, t.toString(), t); - execution.setStatus(ResultStatus.FAILED, HttpResponseStatus.SERVICE_UNAVAILABLE); + state.setStatus(ResultStatus.FAILED, HttpResponseStatus.SERVICE_UNAVAILABLE); } else { - super.handleExecuteStatementException(t, execution); + super.handleExecuteStatementException(t, state, param); } } diff --git a/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java b/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java index 1057a73..56359e3 100644 --- a/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java +++ b/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java @@ -18,6 +18,11 @@ */ package org.apache.asterix.api.http.server; +import static org.apache.asterix.common.exceptions.ErrorCode.ASTERIX; +import static org.apache.asterix.common.exceptions.ErrorCode.QUERY_TIMEOUT; +import static org.apache.asterix.common.exceptions.ErrorCode.REJECT_BAD_CLUSTER_STATE; +import static org.apache.asterix.common.exceptions.ErrorCode.REJECT_NODE_UNREGISTERED; + import java.io.IOException; import java.io.PrintWriter; import java.net.InetAddress; @@ -35,7 +40,6 @@ import org.apache.asterix.common.context.IStorageComponentProvider; import org.apache.asterix.common.dataflow.ICcApplicationContext; import org.apache.asterix.common.exceptions.AsterixException; -import org.apache.asterix.common.exceptions.ErrorCode; import org.apache.asterix.compiler.provider.ILangCompilationProvider; import org.apache.asterix.lang.aql.parser.TokenMgrError; import org.apache.asterix.lang.common.base.IParser; @@ -69,7 +73,6 @@ import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ObjectNode; - import io.netty.handler.codec.http.HttpResponseStatus; public class QueryServiceServlet extends AbstractQueryApiServlet { @@ -212,7 +215,8 @@ on.put("maxResultReads", maxResultReads); return om.writer(new MinimalPrettyPrinter()).writeValueAsString(on); } catch (JsonProcessingException e) { // NOSONAR - return e.getMessage(); + LOGGER.debug("unexpected exception marshalling {} instance to json", getClass(), e); + return e.toString(); } } } @@ -447,7 +451,7 @@ private void handleRequest(IServletRequest request, IServletResponse response) throws IOException { RequestParameters param = getRequestParameters(request); - LOGGER.info(param.toString()); + LOGGER.info("handleRequest: {}", param); long elapsedStart = System.nanoTime(); final PrintWriter httpWriter = response.writer(); @@ -492,7 +496,7 @@ } errorCount = 0; } catch (Exception | TokenMgrError | org.apache.asterix.aqlplus.parser.TokenMgrError e) { - handleExecuteStatementException(e, execution); + handleExecuteStatementException(e, execution, param); response.setStatus(execution.getHttpStatus()); ResultUtil.printError(sessionOutput.out(), e); ResultUtil.printStatus(sessionOutput, execution.getResultStatus()); @@ -531,21 +535,35 @@ execution.end(); } - protected void handleExecuteStatementException(Throwable t, RequestExecutionState execution) { + protected void handleExecuteStatementException(Throwable t, RequestExecutionState state, RequestParameters param) { if (t instanceof org.apache.asterix.aqlplus.parser.TokenMgrError || t instanceof TokenMgrError || t instanceof AlgebricksException) { - GlobalConfig.ASTERIX_LOGGER.log(Level.INFO, t.getMessage(), t); - execution.setStatus(ResultStatus.FATAL, HttpResponseStatus.BAD_REQUEST); - } else if (t instanceof HyracksException) { - GlobalConfig.ASTERIX_LOGGER.log(Level.WARN, t.getMessage(), t); - if (((HyracksException) t).getErrorCode() == ErrorCode.QUERY_TIMEOUT) { - execution.setStatus(ResultStatus.TIMEOUT, HttpResponseStatus.OK); + if (LOGGER.isDebugEnabled()) { + LOGGER.debug("handleException: {}: {}", t.getMessage(), param, t); } else { - execution.setStatus(ResultStatus.FATAL, HttpResponseStatus.INTERNAL_SERVER_ERROR); + LOGGER.info("handleException: {}: {}", t.getMessage(), param); + } + state.setStatus(ResultStatus.FATAL, HttpResponseStatus.BAD_REQUEST); + } else if (t instanceof HyracksException) { + HyracksException he = (HyracksException) t; + switch (he.getComponent() + he.getErrorCode()) { + case ASTERIX + QUERY_TIMEOUT: + LOGGER.info("handleException: query execution timed out: {}", param); + state.setStatus(ResultStatus.TIMEOUT, HttpResponseStatus.OK); + break; + case ASTERIX + REJECT_BAD_CLUSTER_STATE: + case ASTERIX + REJECT_NODE_UNREGISTERED: + LOGGER.warn("handleException: {}: {}", he.getMessage(), param); + state.setStatus(ResultStatus.FATAL, HttpResponseStatus.SERVICE_UNAVAILABLE); + break; + default: + LOGGER.warn("handleException: unexpected exception {}: {}", he.getMessage(), param, he); + state.setStatus(ResultStatus.FATAL, HttpResponseStatus.INTERNAL_SERVER_ERROR); + break; } } else { - GlobalConfig.ASTERIX_LOGGER.log(Level.WARN, "Unexpected exception", t); - execution.setStatus(ResultStatus.FATAL, HttpResponseStatus.INTERNAL_SERVER_ERROR); + LOGGER.warn("handleException: unexpected exception: {}", param, t); + state.setStatus(ResultStatus.FATAL, HttpResponseStatus.INTERNAL_SERVER_ERROR); } } } diff --git a/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/ExecuteStatementRequestMessage.java b/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/ExecuteStatementRequestMessage.java index 31b213e..53d4f3f 100644 --- a/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/ExecuteStatementRequestMessage.java +++ b/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/ExecuteStatementRequestMessage.java @@ -34,6 +34,8 @@ import org.apache.asterix.common.config.GlobalConfig; import org.apache.asterix.common.context.IStorageComponentProvider; import org.apache.asterix.common.dataflow.ICcApplicationContext; +import org.apache.asterix.common.exceptions.ErrorCode; +import org.apache.asterix.common.exceptions.RuntimeDataException; import org.apache.asterix.common.messaging.api.ICcAddressedMessage; import org.apache.asterix.compiler.provider.ILangCompilationProvider; import org.apache.asterix.hyracks.bootstrap.CCApplication; @@ -95,7 +97,7 @@ ClusterControllerService ccSrv = (ClusterControllerService) ccSrvContext.getControllerService(); CCApplication ccApp = (CCApplication) ccSrv.getApplication(); CCMessageBroker messageBroker = (CCMessageBroker) ccSrvContext.getMessageBroker(); - final String rejectionReason = getRejectionReason(ccSrv); + final RuntimeDataException rejectionReason = getRejectionReason(ccSrv); if (rejectionReason != null) { sendRejection(rejectionReason, messageBroker); return; @@ -145,22 +147,22 @@ } } - private String getRejectionReason(ClusterControllerService ccSrv) { + private RuntimeDataException getRejectionReason(ClusterControllerService ccSrv) { if (ccSrv.getNodeManager().getNodeControllerState(requestNodeId) == null) { - return "Node is not registerted with the CC"; + return new RuntimeDataException(ErrorCode.REJECT_NODE_UNREGISTERED); } ICcApplicationContext appCtx = (ICcApplicationContext) ccSrv.getApplicationContext(); IClusterStateManager csm = appCtx.getClusterStateManager(); final IClusterManagementWork.ClusterState clusterState = csm.getState(); if (clusterState != IClusterManagementWork.ClusterState.ACTIVE) { - return "Cannot execute request, cluster is " + clusterState; + return new RuntimeDataException(ErrorCode.REJECT_BAD_CLUSTER_STATE, clusterState); } return null; } - private void sendRejection(String reason, CCMessageBroker messageBroker) { + private void sendRejection(RuntimeDataException reason, CCMessageBroker messageBroker) { ExecuteStatementResponseMessage responseMsg = new ExecuteStatementResponseMessage(requestMessageId); - responseMsg.setError(new Exception(reason)); + responseMsg.setError(reason); try { messageBroker.sendApplicationMessageToNC(responseMsg, requestNodeId); } catch (Exception e) { diff --git a/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/ErrorCode.java b/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/ErrorCode.java index 04054f1..8146797 100644 --- a/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/ErrorCode.java +++ b/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/ErrorCode.java @@ -72,6 +72,8 @@ public static final int UNKNOWN_DURATION_UNIT = 29; public static final int QUERY_TIMEOUT = 30; public static final int INVALID_TYPE_CASTING_MATH_FUNCTION = 31; + public static final int REJECT_BAD_CLUSTER_STATE = 32; + public static final int REJECT_NODE_UNREGISTERED = 33; public static final int INSTANTIATION_ERROR = 100; diff --git a/asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties b/asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties index 1fb8480..f9188b8 100644 --- a/asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties +++ b/asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties @@ -65,6 +65,8 @@ 29 = Unknown duration unit %1$s 30 = Query timed out and will be cancelled 31 = Invalid type-casting math function: %1$s for converting %2$s to %3$s +32 = Cannot execute request, cluster is %1$s +33 = Node is not registered with the CC 100 = Unable to instantiate class %1$s diff --git a/hyracks-fullstack/algebricks/algebricks-common/src/main/java/org/apache/hyracks/algebricks/common/exceptions/AlgebricksException.java b/hyracks-fullstack/algebricks/algebricks-common/src/main/java/org/apache/hyracks/algebricks/common/exceptions/AlgebricksException.java index d1feb08..adb6c79 100644 --- a/hyracks-fullstack/algebricks/algebricks-common/src/main/java/org/apache/hyracks/algebricks/common/exceptions/AlgebricksException.java +++ b/hyracks-fullstack/algebricks/algebricks-common/src/main/java/org/apache/hyracks/algebricks/common/exceptions/AlgebricksException.java @@ -97,6 +97,10 @@ return component; } + public int getErrorCode() { + return errorCode; + } + public Object[] getParams() { return params; } diff --git a/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/HyracksException.java b/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/HyracksException.java index 3aa95b3..c85717d 100644 --- a/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/HyracksException.java +++ b/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/HyracksException.java @@ -136,9 +136,7 @@ @Override public String getMessage() { if (msgCache == null) { - synchronized (this) { - msgCache = ErrorMessageUtil.formatMessage(component, errorCode, super.getMessage(), params); - } + msgCache = ErrorMessageUtil.formatMessage(component, errorCode, super.getMessage(), params); } return msgCache; } -- To view, visit https://asterix-gerrit.ics.uci.edu/2524 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: merged Gerrit-Change-Id: I1f2828481df055d6c96f1ae1869ef37a065bf576 Gerrit-PatchSet: 5 Gerrit-Project: asterixdb Gerrit-Branch: release-0.9.4-pre-rc Gerrit-Owner: Michael Blow <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]>
