Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: delete / cancel query #11708
docs: delete / cancel query #11708
Changes from 1 commit
a01e289
336fe0e
b8a43d1
212fbed
1b53481
a7c0094
ee41c11
390b00a
bd03875
6a83d7b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether we should promote using router instead of broker since the router is now sort of mandatory to use the web console. Or, should we mention that you can use either router or broker address here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The POST examples used BROKER, but I can. change to ROUTER and mention both are options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that this is clearly laid out in a code block, but it's a different format than is used to describe
HTTP POST
, which says: "You can make Druid SQL queries using HTTP via POST to the endpoint/druid/v2/sql/
."Perhaps have a DELETE endpoint description similar to HTTP POST, or edit HTTP POST to have a code block like done here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Queries are allowed to have the same sqlQueryId even when they are different. It would be nice to mention that all queries will be cancelled if you have multiple sql queries running with the same sqlQueryId.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering about that actually. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cancellation request requires the read permission on all resources (currently datasource and view) in the sql query. It returns 404 as well when authorization fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jihoonson Why is status code 404 returned for autorization failure? In compliance with the HTTP status code definition, code 403 is the appropriate one. Actually, the native query endpoint returns 403 for such case
druid/server/src/main/java/org/apache/druid/server/QueryResource.java
Lines 164 to 166 in 7220d04
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @FrankChen021, per the RFC for HTTP, you can return either 403 or 404 (https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.3, https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.4). From security perspective, 404 is preferred to 403 since 403 leaks the existence of resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in such case, this endpoint is already public to everyone, I think there's no concern of security problem so that we need to hide its existence. 404 might be a confusion for users when authorization fails. And it's better that we keep it consistent with the implementation of native query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SQL authorization checks the permission on the resources used in the SQL query, such as datasources or views. Returning 404 is to hide the existence of those datasources or views, not the endpoint. For user confusion, what is the use case where the user should know that he doesn't have permission to cancel the query? The query is usually cancelled by the user who submitted the query or the cluster admin. Either case, they will have the right permission already. For the difference from native query behavior, I'm pretty sure that most clients should handle native queries and sql queries separately anyway since their result format is different.
BUT, I think authorization failure handling should be consistent with the query posting endpoint which returns 403. I will create a PR to fix this. Thank you for bringing this up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raised #11710.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jihoonson