Support real query cancelling for web console #11738
Conversation
ChihyunChang
force-pushed
the
feature-web-console-cancel-query
branch
from
69b4905
to
fd15ac0
Compare
ChihyunChang
force-pushed
the
feature-web-console-cancel-query
branch
from
fd15ac0
to
44d0232
Compare
|
close and re-open to try and trigger travis |
| @@ -196,6 +196,10 @@ export class QueryView extends React.PureComponent<QueryViewProps, QueryViewStat | |||
| return Api.instance.post(`/druid/v2${isSql ? '/sql' : ''}`, payload, { cancelToken }); | |||
| }); | |||
|
|
|||
| const generateQueryId = (): string => { | |||
There was a problem hiding this comment.
I think druid query ids are normally UUID v4 by convention. it would be best to stick to that. Use an off the shelf uuid v4 library like https://www.npmjs.com/package/uuid instead on hand rolling a random number generator here.
There was a problem hiding this comment.
Noted it ! install uuid --save and use it to generate our queryId
| @@ -205,14 +209,23 @@ export class QueryView extends React.PureComponent<QueryViewProps, QueryViewStat | |||
|
|
|||
| const query = QueryView.isJsonLike(queryString) ? Hjson.parse(queryString) : queryString; | |||
|
|
|||
| const queryId = generateQueryId(); | |||
|
|
|||
| const isSql = !QueryView.isJsonLike(queryString); | |||
There was a problem hiding this comment.
no need to call QueryView.isJsonLike(queryString) twice in 3 lines, call it once up top and then reuse the variable
There was a problem hiding this comment.
yes, i should do this. Initialed isSql variable and reuse it in query and delete api url.
| let context: Record<string, any> | undefined; | ||
| if (!isEmptyContext(queryContext) || wrapQueryLimit || mandatoryQueryContext) { | ||
| context = { ...queryContext, ...(mandatoryQueryContext || {}) }; | ||
| context.sqlQueryId = queryId; |
There was a problem hiding this comment.
where are you setting the context.queryId if this is not a SQL query?
There was a problem hiding this comment.
My mistake, i didn't setting the sqlQueryId and it still be able to cancel the query. Added it now
| if (typeof wrapQueryLimit !== 'undefined') { | ||
| context.sqlOuterLimit = wrapQueryLimit + 1; | ||
| } | ||
| } | ||
|
|
||
| void cancelToken.promise.then(() => { | ||
| void Api.instance.delete(`/druid/v2/${isSql ? '/sql' : ''}/${queryId}`); |
There was a problem hiding this comment.
will void here be enough to make sure nothing complains about an unhandled promise rejection (we do not want to trigger https://developer.mozilla.org/en-US/docs/Web/API/Window/unhandledrejection_event)? Can you try this when it fails? Does the error get fully ignored or is there a need for .catch(() => {}) ? If I was in your place I would add the .catch(() => {}) just to be sure (and to save time on doing the proper research)
There was a problem hiding this comment.
yes, you are right, void would not be enough to handle the promise reject. i added catch and throw new DruidError(e). But i did not handle the cancel api response yet.
|
It would be really cool to add a e2e test to this. I am picturing the test opening the query view, typing in a query with a |
yes of course. i will finish our e2e test by the end of this week |
ChihyunChang
force-pushed
the
feature-web-console-cancel-query
branch
from
0a8d94a
to
1c3a0e9
Compare
|
the e2e test passes on my machine but seems to fail in CI, going to investigate why |
…uild nor loaded by default
|
Come on, Travis, you can do it! |
Description
This is a follow up to #11643 When a user presses the cancel query button in the web-console, a query cancel request should be sent to Druid to cancel the already running query
Generating queryId, added queryId in context when submitting POST request.
After cancelToken, check
isSqlthen call DELETE request with the corresponding queryId URLDELETE https://ROUTER:8888/druid/v2/{queryId}DELETE https://ROUTER:8888/druid/v2/sql/{sqlQueryId}Return status
202shows running query deleted successfullyKey changed/added classes in this PR
Druid SQL's HTTP DELETE methodquery-viewThis PR has: