Till Westmann has posted comments on this change. Change subject: Fix async result delivery ......................................................................
Patch Set 28: (23 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 > What is the order of this enum? Should "STARTED" be first if it's executi Done Line 526 > inline? import of IStatementExecutor.ResultDelivery would increase brevity Done 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 > inline? Done https://asterix-gerrit.ics.uci.edu/#/c/1394/28/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 2617: GlobalConfig.ASTERIX_LOGGER.log(Level.SEVERE, > add e to be the last parameter of the log method? Done 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: > Not introduced but arguably exacerbated by this change, should we introduce Done https://asterix-gerrit.ics.uci.edu/#/c/1394/28/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml File asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml: Line 28: <expected-error>HYR0002: Error in processing tuple 0</expected-error> > Should or shouldn't we have error code? Done https://asterix-gerrit.ics.uci.edu/#/c/1394/28/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 821: "no handle for " + reqType + " test " + testFile.toString()); > Use Assert.assert(...)? Done Line 843: throw new IllegalStateException( > Use Assert.assert(...)? Done Line 1091: variableCtx.put("number", 1); > move main(...) to be a unit test? -- a test for TestExecutor..? or remove removed. 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 & i Done https://asterix-gerrit.ics.uci.edu/#/c/1394/28/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/InjectFailureTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/InjectFailureTypeComputer.java: Line 34: if (argIndex == 1 && type.getTypeTag() != ATypeTag.BOOLEAN) { > == 0? The first argument can be anything and will be passed verbatim to the caller. The second argument is a boolean that determines if the invocation throws an exception. Added comment. https://asterix-gerrit.ics.uci.edu/#/c/1394/28/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/SleepDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/SleepDescriptor.java: Line 68: LOGGER.log(Level.INFO, ctx.getTaskAttemptId() + " sleeping for " + time + " ms"); > check log level before the log call? Done Line 73: LOGGER.log(Level.INFO, ctx.getTaskAttemptId() + " done sleeping for " + time + " ms"); > check log level? Done https://asterix-gerrit.ics.uci.edu/#/c/1394/28/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 86: ResultSetId rsId = entry.getKey(); > Rename the method to be "toString"? Done 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 > extra space? Done Line 35 > extra space? Done https://asterix-gerrit.ics.uci.edu/#/c/1394/28/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/dataset/DatasetDirectoryService.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/dataset/DatasetDirectoryService.java: Line 76: LOGGER.info(getClass().getSimpleName() + " notified of new job " + jobId); > Check log level before calling info(...)? Done https://asterix-gerrit.ics.uci.edu/#/c/1394/28/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/dataset/ResultStateSweeper.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/dataset/ResultStateSweeper.java: Line 46: Logger logger) { > What's the motivation of using the logger in DatasetPartitionManager? It w The ResultStateSweeper is used to sweep result state on both the CC and the NC. As the result state that is swept is different in both cases, it seemed useful to align the logging of the sweeper with the logging of the result state owner. Line 62: logger.severe("Result cleaner thread interrupted, shutting down."); > put e as a parameter to the severe call? Done 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 un Done Line 186: @Override > I would prefer this be a synchronized method, since it accesses a thread un Done Line 192: public void deinitState(JobId jobId) { > I would prefer this be a synchronized method, since it accesses a thread un Done Line 197: private void deinit(JobId jobId) { > I would prefer this be a synchronized method, since it accesses a thread un Done -- 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: 28 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
