You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "betodealmeida (via GitHub)" <gi...@apache.org> on 2023/11/07 21:25:45 UTC
[PR] fix: trino cursor [superset]
betodealmeida opened a new pull request, #25897:
URL: https://github.com/apache/superset/pull/25897
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR improves the Trino `execute_with_cursor` method:
1. It uses a `threading.Event` to indicate that the query running in a separate thread has finished, removing the overhead of context switching that happens with `sleep()` calls.
2. More importantly, it removes the call to `query.id` from inside the thread. The `Query` object is loaded lazily, so when `query.id` is accessed we need to use the session to retrieve the ID. In certain deployments the call to get the session inside the thread might fail (at Preset it fails because the thread is missing additional context that the application has).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### 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
Re: [PR] fix: trino cursor [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #25897:
URL: https://github.com/apache/superset/pull/25897#issuecomment-1800318660
@giftig as you wrote the initial implementation, can you also take a look?
--
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 cursor [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #25897:
URL: https://github.com/apache/superset/pull/25897#discussion_r1385774822
##########
superset/db_engine_specs/trino.py:
##########
@@ -193,34 +201,32 @@ def execute_with_cursor(
in another thread and invoke `handle_cursor` to poll for the query ID
to appear on the cursor in parallel.
"""
+ # Fetch the query ID before hand, since it might fail inside the thread due to
+ # how the SQLAlchemy session is handled.
+ query_id = query.id
+
execute_result: dict[str, Any] = {}
+ execute_event = threading.Event()
- def _execute(results: dict[str, Any]) -> None:
- logger.debug("Query %d: Running query: %s", query.id, sql)
+ def _execute(results: dict[str, Any], event: threading.Event) -> None:
+ logger.debug("Query %d: Running query: %s", query_id, sql)
- # Pass result / exception information back to the parent thread
try:
cls.execute(cursor, sql)
- results["complete"] = True
except Exception as ex: # pylint: disable=broad-except
- results["complete"] = True
results["error"] = ex
+ finally:
+ event.set()
- execute_thread = threading.Thread(target=_execute, args=(execute_result,))
+ execute_thread = threading.Thread(
+ target=_execute,
+ args=(execute_result, execute_event),
+ )
execute_thread.start()
+ execute_event.wait()
- # Wait for a query ID to be available before handling the cursor, as
- # it's required by that method; it may never become available on error.
- while not cursor.query_id and not execute_result.get("complete"):
- time.sleep(0.1)
-
- logger.debug("Query %d: Handling cursor", query.id)
- cls.handle_cursor(cursor, query, session)
-
- # Block until the query completes; same behaviour as the client itself
- logger.debug("Query %d: Waiting for query to complete", query.id)
- while not execute_result.get("complete"):
- time.sleep(0.5)
+ logger.debug("Query %d: Handling cursor", query_id)
+ cls.handle_cursor_with_query_id(cursor, query, session, query_id)
Review Comment:
Ah, I didn't know these were different, let me fix this.
--
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 cursor [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #25897:
URL: https://github.com/apache/superset/pull/25897#discussion_r1385774532
##########
superset/db_engine_specs/trino.py:
##########
@@ -193,34 +201,32 @@ def execute_with_cursor(
in another thread and invoke `handle_cursor` to poll for the query ID
to appear on the cursor in parallel.
"""
+ # Fetch the query ID before hand, since it might fail inside the thread due to
+ # how the SQLAlchemy session is handled.
+ query_id = query.id
+
execute_result: dict[str, Any] = {}
+ execute_event = threading.Event()
- def _execute(results: dict[str, Any]) -> None:
- logger.debug("Query %d: Running query: %s", query.id, sql)
+ def _execute(results: dict[str, Any], event: threading.Event) -> None:
+ logger.debug("Query %d: Running query: %s", query_id, sql)
- # Pass result / exception information back to the parent thread
try:
cls.execute(cursor, sql)
- results["complete"] = True
except Exception as ex: # pylint: disable=broad-except
- results["complete"] = True
results["error"] = ex
+ finally:
+ event.set()
- execute_thread = threading.Thread(target=_execute, args=(execute_result,))
+ execute_thread = threading.Thread(
+ target=_execute,
+ args=(execute_result, execute_event),
+ )
execute_thread.start()
+ execute_event.wait()
Review Comment:
Ah, I see. I was confused by the double negative logic `not A and not B`. Let me fix this.
--
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 cursor [superset]
Posted by "giftig (via GitHub)" <gi...@apache.org>.
giftig commented on code in PR #25897:
URL: https://github.com/apache/superset/pull/25897#discussion_r1385689170
##########
superset/db_engine_specs/trino.py:
##########
@@ -193,34 +201,32 @@ def execute_with_cursor(
in another thread and invoke `handle_cursor` to poll for the query ID
to appear on the cursor in parallel.
"""
+ # Fetch the query ID before hand, since it might fail inside the thread due to
+ # how the SQLAlchemy session is handled.
+ query_id = query.id
Review Comment:
Ah, this explains what sqlite was complaining about then; I never did get to the bottom of that.
##########
superset/db_engine_specs/trino.py:
##########
@@ -193,34 +201,32 @@ def execute_with_cursor(
in another thread and invoke `handle_cursor` to poll for the query ID
to appear on the cursor in parallel.
"""
+ # Fetch the query ID before hand, since it might fail inside the thread due to
+ # how the SQLAlchemy session is handled.
+ query_id = query.id
+
execute_result: dict[str, Any] = {}
+ execute_event = threading.Event()
- def _execute(results: dict[str, Any]) -> None:
- logger.debug("Query %d: Running query: %s", query.id, sql)
+ def _execute(results: dict[str, Any], event: threading.Event) -> None:
+ logger.debug("Query %d: Running query: %s", query_id, sql)
- # Pass result / exception information back to the parent thread
try:
cls.execute(cursor, sql)
- results["complete"] = True
except Exception as ex: # pylint: disable=broad-except
- results["complete"] = True
results["error"] = ex
+ finally:
+ event.set()
- execute_thread = threading.Thread(target=_execute, args=(execute_result,))
+ execute_thread = threading.Thread(
+ target=_execute,
+ args=(execute_result, execute_event),
+ )
execute_thread.start()
+ execute_event.wait()
- # Wait for a query ID to be available before handling the cursor, as
- # it's required by that method; it may never become available on error.
- while not cursor.query_id and not execute_result.get("complete"):
- time.sleep(0.1)
-
- logger.debug("Query %d: Handling cursor", query.id)
- cls.handle_cursor(cursor, query, session)
-
- # Block until the query completes; same behaviour as the client itself
- logger.debug("Query %d: Waiting for query to complete", query.id)
- while not execute_result.get("complete"):
- time.sleep(0.5)
+ logger.debug("Query %d: Handling cursor", query_id)
+ cls.handle_cursor_with_query_id(cursor, query, session, query_id)
Review Comment:
You've also passed `query_id` as `query.id`, i.e. the ID of the superset `Query` object, when it should be `cursor.query_id`, the query ID within trino, which we can use to cancel the query.
##########
superset/db_engine_specs/trino.py:
##########
@@ -193,34 +201,32 @@ def execute_with_cursor(
in another thread and invoke `handle_cursor` to poll for the query ID
to appear on the cursor in parallel.
"""
+ # Fetch the query ID before hand, since it might fail inside the thread due to
+ # how the SQLAlchemy session is handled.
+ query_id = query.id
+
execute_result: dict[str, Any] = {}
+ execute_event = threading.Event()
- def _execute(results: dict[str, Any]) -> None:
- logger.debug("Query %d: Running query: %s", query.id, sql)
+ def _execute(results: dict[str, Any], event: threading.Event) -> None:
+ logger.debug("Query %d: Running query: %s", query_id, sql)
- # Pass result / exception information back to the parent thread
try:
cls.execute(cursor, sql)
- results["complete"] = True
except Exception as ex: # pylint: disable=broad-except
- results["complete"] = True
results["error"] = ex
+ finally:
+ event.set()
- execute_thread = threading.Thread(target=_execute, args=(execute_result,))
+ execute_thread = threading.Thread(
+ target=_execute,
+ args=(execute_result, execute_event),
+ )
execute_thread.start()
+ execute_event.wait()
Review Comment:
This waits too long; the point of doing this in a separate thread is that the `cursor.execute` call is only going to return when the query has finished executing, way too late to be calling `handle_cursor`. We poll for the presence of `cursor.query_id` because the `cursor.execute` call is going to make it available once it's received it, but will then block until the query completes.
--
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 cursor [superset]
Posted by "giftig (via GitHub)" <gi...@apache.org>.
giftig commented on code in PR #25897:
URL: https://github.com/apache/superset/pull/25897#discussion_r1385700521
##########
superset/db_engine_specs/trino.py:
##########
@@ -193,34 +201,32 @@ def execute_with_cursor(
in another thread and invoke `handle_cursor` to poll for the query ID
to appear on the cursor in parallel.
"""
+ # Fetch the query ID before hand, since it might fail inside the thread due to
+ # how the SQLAlchemy session is handled.
+ query_id = query.id
+
execute_result: dict[str, Any] = {}
+ execute_event = threading.Event()
- def _execute(results: dict[str, Any]) -> None:
- logger.debug("Query %d: Running query: %s", query.id, sql)
+ def _execute(results: dict[str, Any], event: threading.Event) -> None:
+ logger.debug("Query %d: Running query: %s", query_id, sql)
- # Pass result / exception information back to the parent thread
try:
cls.execute(cursor, sql)
- results["complete"] = True
except Exception as ex: # pylint: disable=broad-except
- results["complete"] = True
results["error"] = ex
+ finally:
+ event.set()
- execute_thread = threading.Thread(target=_execute, args=(execute_result,))
+ execute_thread = threading.Thread(
+ target=_execute,
+ args=(execute_result, execute_event),
+ )
execute_thread.start()
+ execute_event.wait()
- # Wait for a query ID to be available before handling the cursor, as
- # it's required by that method; it may never become available on error.
- while not cursor.query_id and not execute_result.get("complete"):
- time.sleep(0.1)
-
- logger.debug("Query %d: Handling cursor", query.id)
- cls.handle_cursor(cursor, query, session)
-
- # Block until the query completes; same behaviour as the client itself
- logger.debug("Query %d: Waiting for query to complete", query.id)
- while not execute_result.get("complete"):
- time.sleep(0.5)
+ logger.debug("Query %d: Handling cursor", query_id)
+ cls.handle_cursor_with_query_id(cursor, query, session, query_id)
Review Comment:
You've also passed `query_id` as `query.id`, i.e. the ID of the superset `Query` object, when it should be `cursor.query_id`, the query ID within trino, which we can use to cancel the query. I don't think the signature change is necessary though; we're already passing in the cursor so we can take it from there.
--
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 cursor [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida merged PR #25897:
URL: https://github.com/apache/superset/pull/25897
--
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 cursor [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #25897:
URL: https://github.com/apache/superset/pull/25897#issuecomment-1801737926
Thanks @betodealmeida @villebro @giftig @justinpark @sadpandajoe for the investigation and fix. I closed my revert in favor of this PR.
--
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 cursor [superset]
Posted by "giftig (via GitHub)" <gi...@apache.org>.
giftig commented on PR #25897:
URL: https://github.com/apache/superset/pull/25897#issuecomment-1801291819
Thanks @betodealmeida, LGTM now.
--
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