You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "dungdm93 (via GitHub)" <gi...@apache.org> on 2023/08/08 09:21:38 UTC
[GitHub] [superset] dungdm93 opened a new pull request, #24913: fix(trino): cancel_query doesn't abort query execution in Trino server
dungdm93 opened a new pull request, #24913:
URL: https://github.com/apache/superset/pull/24913
Fix #24858
This PR has 2 parts: Superset part - this PR, Trino part:
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
[Screencast from 08-08-2023 16:17:37.webm](https://github.com/apache/superset/assets/6848311/18003690-d8d4-469a-a1cf-ab6a3700530e)
After:
[Screencast from 08-08-2023 16:07:09.webm](https://github.com/apache/superset/assets/6848311/adebf3b7-7bb5-4e95-870f-1630e31dd1e5)
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] giftig commented on a diff in pull request #24913: fix(trino): cancel_query doesn't abort query execution in Trino server
Posted by "giftig (via GitHub)" <gi...@apache.org>.
giftig commented on code in PR #24913:
URL: https://github.com/apache/superset/pull/24913#discussion_r1287069397
##########
superset/db_engine_specs/trino.py:
##########
@@ -178,7 +191,7 @@ def prepare_cancel_query(cls, query: Query, session: Session) -> None:
session.commit()
@classmethod
- def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) -> bool:
+ def cancel_query(cls, cursor: Cursor, query: Query, cancel_query_id: str) -> bool:
Review Comment:
Is it ok to declare the type as `Cursor` here? It's inheriting from the base db engine spec where `cursor` is an `Any` so not sure if this creates a contradiction or if python's type checking rules allows you to extend it and be more specific. I note in other db engine specs like postgres it's also declared as `Any`.
##########
superset/db_engine_specs/trino.py:
##########
@@ -188,11 +201,12 @@ def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) -> bool:
:return: True if query cancelled successfully, False otherwise
"""
try:
+ # Can't use cursor.cancel() because
+ # cursor is new object created in sql_lab.cancel_query
Review Comment:
Not exactly related to your change but I've just realised this method claims to return `True` on success, `False` on failure, but it's actually returning `None` on success, `False` on failure.
Might be a quick win to add a `return True` line while we're here, to fix that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] giftig commented on a diff in pull request #24913: fix(trino): cancel_query doesn't abort query execution in Trino server
Posted by "giftig (via GitHub)" <gi...@apache.org>.
giftig commented on code in PR #24913:
URL: https://github.com/apache/superset/pull/24913#discussion_r1293149929
##########
superset/db_engine_specs/trino.py:
##########
@@ -188,11 +201,12 @@ def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) -> bool:
:return: True if query cancelled successfully, False otherwise
"""
try:
+ # Can't use cursor.cancel() because
+ # cursor is new object created in sql_lab.cancel_query
Review Comment:
Ah, looks like I'm blind.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #24913: fix(trino): cancel_query doesn't abort query execution in Trino server
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24913:
URL: https://github.com/apache/superset/pull/24913#issuecomment-1669255299
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24913?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24913](https://app.codecov.io/gh/apache/superset/pull/24913?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (07dd6ac) into [master](https://app.codecov.io/gh/apache/superset/commit/85a7d5cb3ebe833cfc2980f0846f15bb7ce1dd01?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (85a7d5c) will **decrease** coverage by `10.68%`.
> The diff coverage is `25.00%`.
> :exclamation: Current head 07dd6ac differs from pull request most recent head 2b40022. Consider uploading reports for the commit 2b40022 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24913 +/- ##
===========================================
- Coverage 69.00% 58.32% -10.68%
===========================================
Files 1906 1906
Lines 74134 74144 +10
Branches 8208 8208
===========================================
- Hits 51153 43245 -7908
- Misses 20858 28776 +7918
Partials 2123 2123
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `54.06% <25.00%> (-0.01%)` | :arrow_down: |
| python | `61.03% <25.00%> (-22.34%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `55.04% <25.00%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files Changed](https://app.codecov.io/gh/apache/superset/pull/24913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [superset/db\_engine\_specs/trino.py](https://app.codecov.io/gh/apache/superset/pull/24913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3RyaW5vLnB5) | `73.68% <25.00%> (-10.06%)` | :arrow_down: |
... and [293 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24913/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): cancel_query doesn't abort query execution in Trino server [superset]
Posted by "giftig (via GitHub)" <gi...@apache.org>.
giftig closed pull request #24913: fix(trino): cancel_query doesn't abort query execution in Trino server
URL: https://github.com/apache/superset/pull/24913
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] dungdm93 commented on a diff in pull request #24913: fix(trino): cancel_query doesn't abort query execution in Trino server
Posted by "dungdm93 (via GitHub)" <gi...@apache.org>.
dungdm93 commented on code in PR #24913:
URL: https://github.com/apache/superset/pull/24913#discussion_r1292813838
##########
superset/db_engine_specs/trino.py:
##########
@@ -188,11 +201,12 @@ def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) -> bool:
:return: True if query cancelled successfully, False otherwise
"""
try:
+ # Can't use cursor.cancel() because
+ # cursor is new object created in sql_lab.cancel_query
Review Comment:
Already has `return True` outside of try-catch block
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] giftig commented on a diff in pull request #24913: fix(trino): cancel_query doesn't abort query execution in Trino server
Posted by "giftig (via GitHub)" <gi...@apache.org>.
giftig commented on code in PR #24913:
URL: https://github.com/apache/superset/pull/24913#discussion_r1293149929
##########
superset/db_engine_specs/trino.py:
##########
@@ -188,11 +201,12 @@ def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) -> bool:
:return: True if query cancelled successfully, False otherwise
"""
try:
+ # Can't use cursor.cancel() because
+ # cursor is new object created in sql_lab.cancel_query
Review Comment:
Ah the github diff ended right before it so I must have missed it, I see you're right, looking at the actual file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): cancel_query doesn't abort query execution in Trino server [superset]
Posted by "giftig (via GitHub)" <gi...@apache.org>.
giftig commented on PR #24913:
URL: https://github.com/apache/superset/pull/24913#issuecomment-2298219349
@rusackas we wrote https://github.com/apache/superset/pull/25680 to work around the trino client lib being blocking, but a more "ideal" solution would be to allow the trino client lib to not block and then handle it in a simpler way (this solution).
If these two PRs get to a state where they can be merged it's probably a neater solution. It looks like they've been abandoned though, so unless someone is invested in taking them over we can probably stick with the solution we already have.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): cancel_query doesn't abort query execution in Trino server [superset]
Posted by "giftig (via GitHub)" <gi...@apache.org>.
giftig commented on PR #24913:
URL: https://github.com/apache/superset/pull/24913#issuecomment-2308531048
I'll close this one per above, can always be reopened / adapted if changes go into the trino library to allow us simpler usage here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(trino): cancel_query doesn't abort query execution in Trino server [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #24913:
URL: https://github.com/apache/superset/pull/24913#issuecomment-2297542970
The linked Trino PR still hasn't been merged, but the linked Superset Issue has been closed as resolved by https://github.com/apache/superset/pull/25680. Should we close this, or is there more to unpack here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] dungdm93 commented on a diff in pull request #24913: fix(trino): cancel_query doesn't abort query execution in Trino server
Posted by "dungdm93 (via GitHub)" <gi...@apache.org>.
dungdm93 commented on code in PR #24913:
URL: https://github.com/apache/superset/pull/24913#discussion_r1288028040
##########
superset/db_engine_specs/trino.py:
##########
@@ -178,7 +191,7 @@ def prepare_cancel_query(cls, query: Query, session: Session) -> None:
session.commit()
@classmethod
- def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) -> bool:
+ def cancel_query(cls, cursor: Cursor, query: Query, cancel_query_id: str) -> bool:
Review Comment:
`Cursor` here is trino specific Cursor. `BaseEngineSpec` define cursor as `Any` because each database driver has different `Cursor` class.
If's safe to be used here, see other methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org