Michael Blow has posted comments on this change. Change subject: Fix async result delivery ......................................................................
Patch Set 30: (16 comments) https://asterix-gerrit.ics.uci.edu/#/c/1394/30/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java: Line 156: STARTED("started"), What is the order of this enum? Should "STARTED" be first if it's execution order? Line 526: final boolean asyncDelivery = IStatementExecutor.ResultDelivery.ASYNC == delivery; inline? import of IStatementExecutor.ResultDelivery would increase brevity... https://asterix-gerrit.ics.uci.edu/#/c/1394/30/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryStatusApiServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryStatusApiServlet.java: Line 96: String status = resultReader.getStatus().name(); inline? https://asterix-gerrit.ics.uci.edu/#/c/1394/30/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java: Line 2611: threadFactory.newThread(() -> { Why thread and not just provide a Callable to an ExecutorService? https://asterix-gerrit.ics.uci.edu/#/c/1394/30/asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/CCApplicationEntryPoint.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/CCApplicationEntryPoint.java: Line 243: ccExtensionManager.getStatementExecutorFactory(appCtx.getThreadFactory())); Not introduced but arguably exacerbated by this change, should we introduce a method for ccExtensionManager.getStatementExecutorFactory(appCtx.getThreadFactory()) ? https://asterix-gerrit.ics.uci.edu/#/c/1394/30/asterixdb/asterix-common/src/test/java/org/apache/asterix/test/aql/TestExecutor.java File asterixdb/asterix-common/src/test/java/org/apache/asterix/test/aql/TestExecutor.java: Line 392: if (!parentDir.isDirectory() && !parentDir.mkdirs()) { Should this be || or else add another error for if the parentDir exists & isn't a directory? https://asterix-gerrit.ics.uci.edu/#/c/1394/30/asterixdb/asterix-installer/src/test/java/org/apache/asterix/installer/test/ManagixSqlppExecutionIT.java File asterixdb/asterix-installer/src/test/java/org/apache/asterix/installer/test/ManagixSqlppExecutionIT.java: Line 32: @Parameters(name = "ManagixSqlppExecutionIT {index}: {0}") amen! https://asterix-gerrit.ics.uci.edu/#/c/1394/30/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataset/DatasetJobRecord.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataset/DatasetJobRecord.java: Line 63: public void fail(ResultSetId rsId, int partition) { getDirectoryRecord() assumes single-threaded execution- should this method be documented as such and made package-private? Line 114: public DatasetDirectoryRecord getDirectoryRecord(ResultSetId rsId, int partition) { getDirectoryRecord() assumes single-threaded execution- should this method be documented as such and made package-private? Line 122: public void updateStatus(ResultSetId rsId) { updateStatus() assumes single-threaded execution- should this method be documented as such and made package-private? https://asterix-gerrit.ics.uci.edu/#/c/1394/30/hyracks-fullstack/hyracks/hyracks-api/src/main/resources/errormsg/en.properties File hyracks-fullstack/hyracks/hyracks-api/src/main/resources/errormsg/en.properties: Line 34: 13=Failure producing result set %1$s for job %2$s extra space? Line 35: 14=No exception for failed result set %1$s for job %2$s extra space? https://asterix-gerrit.ics.uci.edu/#/c/1394/30/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/dataset/DatasetPartitionManager.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/dataset/DatasetPartitionManager.java: Line 182: public Set<JobId> getJobIds() { I would prefer this be a synchronized method, since it accesses a thread unsafe map, even though all current callers synchronize on this. Line 186: @Override I would prefer this be a synchronized method, since it accesses a thread unsafe map, even though all current callers synchronize on this. Line 192: public void deinitState(JobId jobId) { I would prefer this be a synchronized method, since it accesses a thread unsafe map, even though all current callers synchronize on this. Line 197: private void deinit(JobId jobId) { I would prefer this be a synchronized method, since it accesses a thread unsafe map, even though all current callers synchronize on this. -- To view, visit https://asterix-gerrit.ics.uci.edu/1394 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iafba65d9c7bd8643c42e5126c8d89164ae328908 Gerrit-PatchSet: 30 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Till Westmann <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-HasComments: Yes
