asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yingyi Bu (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Add a REST endpoint for query cancellation.
Date Mon, 13 Mar 2017 07:03:11 GMT
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 <buyingyi@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Michael Blow <mblow@apache.org>
Gerrit-Reviewer: Steven Jacobs <sjaco002@ucr.edu>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message