>From Murtadha Hubail <[email protected]>: Murtadha Hubail has submitted this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21283?usp=email )
Change subject: [ASTERIXDB-3649][HYR][API] Call job record finish before callback ...................................................................... [ASTERIXDB-3649][HYR][API] Call job record finish before callback - user model changes: no - storage format changes: no - interface changes: no Details: - ensure calling job record finish to record job end time before calling the result callback since the result callback will use the startTime, endTime, ... of the job record. - Make elapsedTime for result REST API be the time spent delivering the result. Ext-ref: MB-71997 Change-Id: I8f2a79d24f06258c6444a6cfaa531223e465c23e Reviewed-on: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21283 Integration-Tests: Jenkins <[email protected]> Reviewed-by: Ali Alsuliman <[email protected]> Tested-by: Jenkins <[email protected]> Reviewed-by: Murtadha Hubail <[email protected]> --- 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/NCQueryResultApiServlet.java M asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCQueryStatusApiServlet.java M asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryResultApiServlet.java M hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/result/ResultJobRecord.java M hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/result/ResultDirectoryService.java 6 files changed, 21 insertions(+), 16 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-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 e8c8a61..7d6791a 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 @@ -196,8 +196,7 @@ return null; } try { - if (handle.getRequestId() != null - && !isValidRequest(handle.getRequestId(), handle.getJobId(), request, response)) { + if (!isValidRequest(handle.getRequestId(), handle.getJobId(), request, response)) { return null; } } catch (HyracksDataException e) { @@ -209,6 +208,10 @@ protected boolean isValidRequest(String requestId, JobId jobId, IServletRequest request, IServletResponse response) throws HyracksDataException { + if (requestId == null) { + // for backward compatibility, if requestId is not provided, we assume it's a valid request + return true; + } Optional<IClientRequest> clientRequest = ((ICcApplicationContext) appCtx).getRequestTracker().getAsyncOrDeferredRequest(requestId); if (clientRequest.isEmpty() || ((ClientRequest) clientRequest.get()).getJobId() == null) { diff --git a/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCQueryResultApiServlet.java b/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCQueryResultApiServlet.java index 1340893..986ada0 100644 --- a/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCQueryResultApiServlet.java +++ b/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCQueryResultApiServlet.java @@ -40,7 +40,8 @@ @Override protected boolean isValidRequest(String requestId, JobId jobId, IServletRequest request, IServletResponse response) throws HyracksDataException { - return AsyncRequestsAPIUtil.isValidRequest(appCtx, requestId, jobId, response); + // for backward compatibility, if requestId is not provided, we assume it's a valid request + return requestId == null || AsyncRequestsAPIUtil.isValidRequest(appCtx, requestId, jobId, response); } @Override diff --git a/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCQueryStatusApiServlet.java b/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCQueryStatusApiServlet.java index cb77fb0..f4c8236 100644 --- a/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCQueryStatusApiServlet.java +++ b/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCQueryStatusApiServlet.java @@ -44,7 +44,8 @@ protected boolean isValidRequest(String requestId, JobId jobId, IServletRequest request, IServletResponse response) throws HyracksDataException { - return AsyncRequestsAPIUtil.isValidRequest(appCtx, requestId, jobId, response); + // for backward compatibility, if requestId is not provided, we assume it's a valid request + return requestId == null || AsyncRequestsAPIUtil.isValidRequest(appCtx, requestId, jobId, response); } public void printMetricsWithoutResultMetadata(ResponsePrinter printer, IServletRequest request, String requestId, diff --git a/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryResultApiServlet.java b/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryResultApiServlet.java index 97a4b7f..01ee878 100644 --- a/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryResultApiServlet.java +++ b/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryResultApiServlet.java @@ -20,7 +20,6 @@ import java.io.IOException; import java.util.concurrent.ConcurrentMap; -import java.util.concurrent.TimeUnit; import org.apache.asterix.app.result.ResponseMetrics; import org.apache.asterix.app.result.ResponsePrinter; @@ -145,11 +144,13 @@ if (metadata.getFormat() == SessionConfig.OutputFormat.CLEAN_JSON || metadata.getFormat() == SessionConfig.OutputFormat.LOSSLESS_JSON || metadata.getFormat() == SessionConfig.OutputFormat.LOSSLESS_ADM_JSON) { + long resultDeliveryStart = System.nanoTime(); printer.begin(); printStarted = true; printer.addResultPrinter(new ResultsPrinter(appCtx, resultReader, null, stats, sessionOutput)); printer.printResults(); - ResponseMetrics metrics = buildMetrics(stats, metadata); + long resultDeliveryElapsed = System.nanoTime() - resultDeliveryStart; + ResponseMetrics metrics = buildMetrics(stats, metadata, resultDeliveryElapsed); printer.addFooterPrinter(new MetricsPrinter(metrics, HttpUtil.getPreferredCharset(request))); if (metadata.getJobProfile() != null) { printer.addFooterPrinter(new ProfilePrinter(metadata.getJobProfile())); @@ -170,8 +171,7 @@ } } - private ResponseMetrics buildMetrics(Stats stats, ResultMetadata metadata) { - long endTime = System.currentTimeMillis(); + private ResponseMetrics buildMetrics(Stats stats, ResultMetadata metadata, long resultDeliveryElapsed) { stats.setProcessedObjects(metadata.getProcessedObjects()); stats.setQueueWaitTimeNanos(metadata.getQueueWaitTimeNanos()); stats.setBufferCacheHitRatio(metadata.getBufferCacheHitRatio()); @@ -180,11 +180,10 @@ stats.setCloudPagesReadCount(metadata.getCloudPagesReadCount()); stats.setCloudPagesPersistedCount(metadata.getCloudPagesPersistedCount()); stats.updateTotalWarningsCount(metadata.getTotalWarningsCount()); - return ResponseMetrics.of(TimeUnit.MILLISECONDS.toNanos(endTime - metadata.getCreateTimeMillis()), - metadata.getJobDuration(), stats.getCount(), stats.getSize(), metadata.getProcessedObjects(), 0, - metadata.getTotalWarningsCount(), metadata.getCompileTimeNanos(), stats.getQueueWaitTimeNanos(), - stats.getBufferCacheHitRatio(), stats.getBufferCachePageReadCount(), stats.getCloudReadRequestsCount(), - stats.getCloudPagesReadCount(), stats.getCloudPagesPersistedCount()); + return ResponseMetrics.of(resultDeliveryElapsed, metadata.getJobDuration(), stats.getCount(), stats.getSize(), + metadata.getProcessedObjects(), 0, metadata.getTotalWarningsCount(), metadata.getCompileTimeNanos(), + stats.getQueueWaitTimeNanos(), stats.getBufferCacheHitRatio(), stats.getBufferCachePageReadCount(), + stats.getCloudReadRequestsCount(), stats.getCloudPagesReadCount(), stats.getCloudPagesPersistedCount()); } /** diff --git a/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/result/ResultJobRecord.java b/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/result/ResultJobRecord.java index a9a0492..692b4b5 100644 --- a/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/result/ResultJobRecord.java +++ b/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/result/ResultJobRecord.java @@ -118,6 +118,7 @@ } public void finishWithStatus(JobStatus jobStatus) { + finish(); if (jobStatus != null && (status.state == State.RUNNING || status.state == State.IDLE)) { switch (jobStatus) { case TERMINATED -> updateState(State.SUCCESS); diff --git a/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/result/ResultDirectoryService.java b/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/result/ResultDirectoryService.java index 40643d4..c07b9ec 100644 --- a/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/result/ResultDirectoryService.java +++ b/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/result/ResultDirectoryService.java @@ -113,11 +113,11 @@ resultJobRecord.finishWithStatus(jobStatus); jobResultCallback.completed(jobId, resultJobRecord); } else { + if (resultJobRecord != null) { + resultJobRecord.finish(); + } reportJobFailure(jobId, exceptions, resultJobRecord); } - if (resultJobRecord != null) { - resultJobRecord.finish(); - } } private ResultJobRecord getResultJobRecord(JobId jobId) { -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21283?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: lumina Gerrit-Change-Id: I8f2a79d24f06258c6444a6cfaa531223e465c23e Gerrit-Change-Number: 21283 Gerrit-PatchSet: 3 Gerrit-Owner: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]>
