Yingyi Bu has posted comments on this change. Change subject: Add a REST endpoint for query cancellation. ......................................................................
Patch Set 13: (20 comments) https://asterix-gerrit.ics.uci.edu/#/c/1564/13/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/ctx/StatementExecutorContext.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/ctx/StatementExecutorContext.java: PS13, Line 30: activeQueries > The term "active" is overloaded in AsterixDB. I think that it'd be good to Done https://asterix-gerrit.ics.uci.edu/#/c/1564/13/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryCancellationServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryCancellationServlet.java: PS13, Line 51: setContentType > I think that we don't return content, so we don't need a content type. Done PS13, Line 60: activeQueries > Again, the term "active" is overloaded in AsterixDB. I think that it'd be g Done PS13, Line 61: ACTIVE_QUERIES_ATTR > see above Done https://asterix-gerrit.ics.uci.edu/#/c/1564/13/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/RestApiServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/RestApiServlet.java: PS13, Line 206: null, null) > these seem to be null most of the time. maybe consider overloading? Done https://asterix-gerrit.ics.uci.edu/#/c/1564/13/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/ServletConstants.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/ServletConstants.java: PS13, Line 25: EXECUTOR_SERVICE > this seems to be named differently from other attr variables. make them con Done PS13, Line 26: ACTIVE_QUERIES_ATTR > Again, the term "active" is overloaded in AsterixDB. I think that it'd be g Done https://asterix-gerrit.ics.uci.edu/#/c/1564/13/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/java/AsterixJavaClient.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/java/AsterixJavaClient.java: PS13, Line 108: null, null > yet another place where we pass nulls. Done https://asterix-gerrit.ics.uci.edu/#/c/1564/13/asterixdb/asterix-app/src/test/java/org/apache/asterix/api/http/servlet/QueryCancellationServletTest.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/api/http/servlet/QueryCancellationServletTest.java: PS13, Line 47: testGet( > s/testGet/testDelete/? Done https://asterix-gerrit.ics.uci.edu/#/c/1564/13/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/CancellationTestExecutor.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/CancellationTestExecutor.java: PS13, Line 70: invest > s/invest/investigate/? Done PS13, Line 75: submit > s/submit/submitted/ Done https://asterix-gerrit.ics.uci.edu/#/c/1564/13/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/ResultExtractor.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/ResultExtractor.java: PS13, Line 62: "clientContextID" > make those constants? so if needed, they only need to change in a single lo 1. Those constant strings only appear once in this class. 2. It's a switch case statements, each case requires to be a constant. https://asterix-gerrit.ics.uci.edu/#/c/1564/13/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/TestExecutor.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/TestExecutor.java: PS13, Line 1228: if (result == null) { : return; : } > how can result be null for select dataverses? Done https://asterix-gerrit.ics.uci.edu/#/c/1564/13/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/SqlppExecutionWithCancellationTest.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/SqlppExecutionWithCancellationTest.java: PS13, Line 34: with the storage parallelism > fix comment Done https://asterix-gerrit.ics.uci.edu/#/c/1564/13/asterixdb/asterix-app/src/test/resources/runtimets/queries/types/any-object/any-object.2.query.aql File asterixdb/asterix-app/src/test/resources/runtimets/queries/types/any-object/any-object.2.query.aql: PS13, Line 21: where $x.DataverseName = "test" > why the additional condition? Every read-only query is cancelled in SqlppExecutionWithCancellationTest. Since the current cancel() implementation is not synchronous, there could be failed drop dataverse statements (because some dataset is still in use), which then lead to more things in metadata.. Adding a "test" filter still tests the things that were tested before. https://asterix-gerrit.ics.uci.edu/#/c/1564/2/asterixdb/asterix-app/src/test/resources/runtimets/results/types/any-object/any-object.2.adm File asterixdb/asterix-app/src/test/resources/runtimets/results/types/any-object/any-object.2.adm: > the old result is ordered correctly but the new one is not? The order by attributes had a typo in patchset 2: order by DataversName, DatatypeName; I have fixed that in later patches and finally removed order by since metadata dataset only has 1 partition. https://asterix-gerrit.ics.uci.edu/#/c/1564/13/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/utils/Servlets.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/utils/Servlets.java: PS13, Line 37: query > To be more consistent with the other endpoints I'd not prefix this admin en Done PS13, Line 37: QUERY_CANCEL = "/query/admin/active_requests > should this be renamed? Done https://asterix-gerrit.ics.uci.edu/#/c/1564/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMPrimaryUpsertOperatorNodePushable.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMPrimaryUpsertOperatorNodePushable.java: PS13, Line 319: indexHelper.close(); > if index is not assigned in open(), then indexHelper wasn't opened. add a c 1. I didn't change this file in patch set 13. I changed it in patch set 2 but then reverted it. 2. close() should only be called when open is completed successfully? Otherwise, we also need to check whether cursor and writer are opened. https://asterix-gerrit.ics.uci.edu/#/c/1564/13/hyracks-fullstack/hyracks/hyracks-dataflow-common/src/main/java/org/apache/hyracks/dataflow/common/io/RunFileReader.java File hyracks-fullstack/hyracks/hyracks-dataflow-common/src/main/java/org/apache/hyracks/dataflow/common/io/RunFileReader.java: PS13, Line 88: idempotent > This is only safe, if each handle is only handled by a single thread. Is th Yes, this class is not thread-safe. In the system, a run file reader should only be used by a single thread. -- To view, visit https://asterix-gerrit.ics.uci.edu/1564 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2936ac83f71bbef533e2695ed0a2b220c23fc483 Gerrit-PatchSet: 13 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Yingyi Bu <buyin...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Michael Blow <mb...@apache.org> Gerrit-Reviewer: Steven Jacobs <sjaco...@ucr.edu> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
