villebro opened a new issue, #29515: URL: https://github.com/apache/superset/issues/29515
## [SIP] Proposal for Simplified Global Async Queries ### Motivation With [\[SIP-39\] Global Async Query Support](#9190) (GAQ for short) still being behind an experimental feature flag, and not actively maintained, I've been thinking about ways we could simplify the architecture, and finally make this generally available in a forthcoming Superset release. I feel the following issues have all done their part to contribute to not gain wide community traction: - **No deduplication support**: In my experience, the lack of deduplication of heavy queries is one of the main bottlenecks of Superset, and tends to cause major issues when many people try to access the same charts/dashboards. There was an effort to add [deduplication support to GAQ](#14112), but the proposal went stale. - **Design complexity**: There was heated discussion during the SIP process about websockets potentially being too heavy handed for this particular task, with channels, reconnection functionality etc. In retrospect, I feel most of the raised concerns comments turned out to be true - while the implementation may be more elegant than a simple polling solution, very few committers or community members ended up fully understanding the minutiae of the implementation, causing it to fall out of active maintenance. - **New components**: Using the websocket feature required adding a new component to the Superset deployment, along with tightly coupling with the Redis Streams protocol. A simpler polling solution would likely have received more adoption from the community, leading to the feature stabilizing more quickly. Having said all this, the feature is still as relevant today as it was when the original SIP was opened, and I think stabilizing this feature is very important is because Superset's current synchronous query execution model causes lots of issues: - If many people open the same chart/dashboard at the same time, they will all start a query to the underlying database, due to no locking of queries - if a user refreshes a dashboard multiple times, they can quickly congest the downstream database with heavy queries, both eating up webserver threads and database resources. - There's no way to cancel queries that get orphaned by closed browsers. - In some cases, the web worker threads/processes get blocked waiting for long running queries to complete executing, making it impossible to effectively scale web worker replica sets based on CPU consumption. By moving queries to async workers it should become possible to get by with much slimmer webworker replica sets. Furthermore, async workers could be scaled up/down based on the queue depth. ### Proposed Change To simplify the architecture and reuse existing functionality, I propose the following: - The websocket architecture is removed, as it adds a lot of complexity to the architecture - in the future only polling would be supported. - The concept of a "query context cache key" is removed in favor of only a single cache key, i.e. the one we already use for chart data. - A new model is introduced for async queries. If data for a particular cache key isn't found, an entry is added to the new model, which tracks the query progress. The model will get a menu in the UI, from which users will be able to cancel their own queries (Admins will see all queries). Ultimately, this entry gets deleted once the data request is completed. - When requesting chart data, if the data exists in the cache, the data is returned normally. When chart data isn't available in the cache, only the cache_key is returned, along with additional details: when the most recent chart data request has been submitted, status (pending, executing), last heartbeat from the async worker etc. The async execution flow is changed to be similar to SQL Lab async execution, with the following changes: - when the async worker starts executing the query, the cache key is locked using the KeyValueDistributedLock context manager. This means that only a single worker executes any one cache key query at a time. - To support automatic cancellation of queries, we add a new optional field `poll_ttl` to the query context, which makes it possible to automatically cancel queries that are not being actively polled. Every time the cache key is polled, the latest poll time is updated on the metadata object. While executing, the worker periodically checks the metadata object, and if the `poll_ttl` is defined, and if the last poll time exceeds the TTL, the query is cancelled. This ensures that if a person closes a dashboard with lots of long running queries, the queries are automatically cancelled if nobody is actively waiting for the results. By default, frontend requests have poll_ttl set to whichever value is set in the config (`DEFAULT_CHART_DATA_POLL_TTL`). Cache warmup requests would likely not have a `poll_ttl` set, so as to avoid unnecessary polling. - To limit hammering the polling endpoint, we introduce a customizable backoff function in `superset_config.py`, which makes it possible to define how polling backoff should be implemented. The default behavior would be some sort of exponential backoff, where freshly started queries are polled more actively, and queries that have been pending/running for a long time are polled less frequently. When the frontend requests chart data, the backend provides the recommended wait time in the response based on the backoff function. Note, that backoff will be based on time passed since query submission time; this means, that if I open a dashboard with a chart that has a query that's been running for 10 minutes, the browser will repoll much slower than it would if the query would have been dispatched to the async workers right away Some random thoughts: - Currently multi-query query contexts get executed serially. With this new approach the queries can be executed in parallel, as each query is dispatched separately. - I feel synchronous execution is very problematic in the context of Superset due to the problems described in the intro of this post. Originally I thought about proposing making async queries the *only* supported query mechanism in Superset. However, as @betodealmeida [pointed out](https://github.com/apache/superset/issues/9190#issuecomment-2206778896), certain databases that are expected to return data at sub second latencies are better suited to a synchronous flow, as dispatching Celery tasks can add a few seconds of extra overhead to the process. For this reason, we should probably keep async execution an optional feature. ### New or Changed Public Interfaces - New config flag for providing a backoff function for chart data polling. This will used to tell the frontend when it should poll for completed chart data the next time. - New config flag for setting the default chart data poll TTL. - New FAB model + DAO for async queries, along with a new menu for managing currently queued/executing queries. - New optional fields added to chart data request: - `poll_ttl`: if set, the query will be cancelled unless a client has asked for data within the TTL bounds. This will ensure that dashboards that are closed don't leave orphaned chart data requests. - `execution_mode`: the client can ask the query to be executed sync, async or using the default mode. This is specifically added for programmatic integrations, where implementing the polling mechanism may sometimes add unnecessary complexity. - New fields added to the chart data response: - `poll_delay`: how many seconds should the client wait before checking if the query has completed. This value will be calculated by the backend based on the backoff function. - `status` and `start_dttm`: is the query queued or started, and when did the query start executing. This information can be used by the chart component to give the user information of the state of the query, similar to how we currently display how stale the cached data is. So in the future, we may display the following: "Query is queued", or "Query executing for 2 minutes". ### New dependencies In the base proposal, I suggest not adding any new dependencies, and simply supporting polling. However, we may consider using [Server-sent events](https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events) as noted by @betodealmeida . ### Migration Plan and Compatibility 1. Remove feature flag + related code, Websocket worker code 2. Remove references to websocket deployment/service from Helm chart 3. Add new feature flag `SIMPLIFIED_GLOBAL_ASYNC_QUERIES` ### Rejected Alternatives - For now I suggest not implementing `Server-sent events` to keep the implementation simple. -- 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-unsubscr...@superset.apache.org.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org