>From Ali Alsuliman <[email protected]>: Ali Alsuliman has submitted this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20509?usp=email )
Change subject: [ASTERIXDB-3485][OTH] improvements to client request cancellation ...................................................................... [ASTERIXDB-3485][OTH] improvements to client request cancellation - user model changes: no - storage format changes: no - interface changes: yes Ext-ref: MB-63174 Change-Id: Ic94f4a87e93636ed113ffc1ed3ce2ed742bb0276 Reviewed-on: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20509 Reviewed-by: Ali Alsuliman <[email protected]> Integration-Tests: Jenkins <[email protected]> Tested-by: Jenkins <[email protected]> Reviewed-by: Murtadha Hubail <[email protected]> --- M asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/BaseClientRequest.java M asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractQueryApiServlet.java M asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ActiveRequestsServlet.java M asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/CancelQueryRequest.java M asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IClientRequest.java M asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IRequestTracker.java M asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties M asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/utils/RequestTracker.java 8 files changed, 47 insertions(+), 19 deletions(-) Approvals: Ali Alsuliman: Looks good to me, but someone else must approve Jenkins: Verified; Verified Murtadha Hubail: Looks good to me, approved diff --git a/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/BaseClientRequest.java b/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/BaseClientRequest.java index e03e330..f49f585 100644 --- a/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/BaseClientRequest.java +++ b/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/BaseClientRequest.java @@ -50,15 +50,18 @@ } @Override - public synchronized void cancel(ICcApplicationContext appCtx) throws HyracksDataException { + public synchronized boolean cancel(ICcApplicationContext appCtx) throws HyracksDataException { if (complete) { - return; + // it should also be true that the request has already been untracked by the RequestTracker + return false; } if (cancellable) { complete(); state = State.CANCELLED; doCancel(appCtx); + return true; } + return false; } @Override @@ -67,6 +70,15 @@ } @Override + public synchronized boolean markUncancellable() { + if (state == State.CANCELLED) { + return false; + } + cancellable = false; + return true; + } + + @Override public synchronized boolean isCancelled() { return state == State.CANCELLED; } diff --git a/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractQueryApiServlet.java b/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractQueryApiServlet.java index 06c83cd..e8c8a61 100644 --- a/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractQueryApiServlet.java +++ b/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractQueryApiServlet.java @@ -161,6 +161,9 @@ logException(Level.INFO, "request execution timed out", requestId, clientContextId); executionState.setStatus(ResultStatus.TIMEOUT, HttpResponseStatus.OK); return true; + case REQUEST_CANCELLED: + executionState.setStatus(ResultStatus.FAILED, HttpResponseStatus.INTERNAL_SERVER_ERROR); + return true; case REJECT_NODE_UNREGISTERED: case REJECT_BAD_CLUSTER_STATE: logException(Level.WARN, ex.getMessage(), requestId, clientContextId); diff --git a/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ActiveRequestsServlet.java b/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ActiveRequestsServlet.java index 3f1845e..2b5d2cc 100644 --- a/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ActiveRequestsServlet.java +++ b/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ActiveRequestsServlet.java @@ -72,9 +72,12 @@ } try { // Cancels the on-going job. - requestTracker.cancel(req.getId()); - // response: OK - response.setStatus(HttpResponseStatus.OK); + if (requestTracker.cancel(req.getId())) { + // response: OK + response.setStatus(HttpResponseStatus.OK); + } else { + response.setStatus(HttpResponseStatus.FORBIDDEN); + } } catch (Exception e) { LOGGER.log(Level.WARN, "unexpected exception thrown from cancel", e); // response: INTERNAL SERVER ERROR diff --git a/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/CancelQueryRequest.java b/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/CancelQueryRequest.java index 6570041..26277ca 100644 --- a/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/CancelQueryRequest.java +++ b/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/CancelQueryRequest.java @@ -61,8 +61,11 @@ } else { try { requestId = req.getId(); - requestTracker.cancel(requestId); - status = RequestStatus.SUCCESS; + if (requestTracker.cancel(requestId)) { + status = RequestStatus.SUCCESS; + } else { + status = RequestStatus.REJECTED; + } } catch (Exception e) { LOGGER.log(Level.WARN, "unexpected exception thrown from cancel", e); status = RequestStatus.FAILED; diff --git a/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IClientRequest.java b/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IClientRequest.java index ee518dd..2d239c1 100644 --- a/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IClientRequest.java +++ b/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IClientRequest.java @@ -81,6 +81,13 @@ void markCancellable(); /** + * Mark the request as not cancellable if it has not been cancelled yet. + * + * @return true if the request was marked as not cancellable. Otherwise, false without changing anything. + */ + boolean markUncancellable(); + + /** * @return true if the request can be cancelled. Otherwise false. */ boolean isCancellable(); @@ -91,7 +98,7 @@ * @param appCtx * @throws HyracksDataException */ - void cancel(ICcApplicationContext appCtx) throws HyracksDataException; + boolean cancel(ICcApplicationContext appCtx) throws HyracksDataException; /** * @return A json string representation of this request diff --git a/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IRequestTracker.java b/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IRequestTracker.java index dbae70e..b27a4e0 100644 --- a/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IRequestTracker.java +++ b/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IRequestTracker.java @@ -55,7 +55,7 @@ * @param requestId * @throws HyracksDataException */ - void cancel(String requestId) throws HyracksDataException; + boolean cancel(String requestId) throws HyracksDataException; /** * Completes the request with id {@code requestId} 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 9c875d0..5cff16c 100644 --- a/asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties +++ b/asterixdb/asterix-common/src/main/resources/asx_errormsg/en.properties @@ -75,7 +75,7 @@ 38 = Input contains different list types 39 = Expected integer value, got %1$s 40 = No statement provided -41 = Request %1$s has been cancelled +41 = Request has been cancelled %1$s 42 = %1$s: '%2$s' is not a TPC-DS table name 43 = Value out of range, function %1$s expects its %2$s input parameter value to be between %3$s and %4$s, received %5$s 44 = %1$s statement is not supported in read-only mode diff --git a/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/utils/RequestTracker.java b/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/utils/RequestTracker.java index 24bbd06..1dbb6b2 100644 --- a/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/utils/RequestTracker.java +++ b/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/utils/RequestTracker.java @@ -101,15 +101,12 @@ } @Override - public void cancel(String requestId) throws HyracksDataException { + public boolean cancel(String requestId) throws HyracksDataException { final IClientRequest request = runningRequests.get(requestId); if (request == null) { - return; + return false; } - if (!request.isCancellable()) { - throw new IllegalStateException("Request " + request.getId() + " cannot be cancelled"); - } - cancel(request); + return cancel(request); } @Override @@ -131,9 +128,12 @@ return Collections.unmodifiableCollection(new ArrayList<>(completedRequests.values())); } - private void cancel(IClientRequest request) throws HyracksDataException { - request.cancel(ccAppCtx); - untrack(request); + private boolean cancel(IClientRequest request) throws HyracksDataException { + boolean cancelled = request.cancel(ccAppCtx); + if (cancelled) { + untrack(request); + } + return cancelled; } private void untrack(IClientRequest request) { -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20509?usp=email To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: asterixdb Gerrit-Branch: phoenix Gerrit-Change-Id: Ic94f4a87e93636ed113ffc1ed3ce2ed742bb0276 Gerrit-Change-Number: 20509 Gerrit-PatchSet: 3 Gerrit-Owner: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Peeyush Gupta <[email protected]>
