[GitHub] [incubator-superset] robdiciuccio edited a comment on pull request #11499: feat(SIP-39): Async query support for charts

[GitBox]

robdiciuccio edited a comment on pull request #11499:
URL: 
https://github.com/apache/incubator-superset/pull/11499#issuecomment-742018892


   > Managed to make it work but I feel the round trip for the polling might 
have made the page appear slower, especially when cache already exists. Can we 
let explore_json and chart/data API return the cached query results directly 
when the cache key exists?
   
   I did some performance testing on my local machine this morning with the 
"USA Birth Names" example dashboard, and the timing differences are fairly 
negligible (see below).
   
   **Async:**
   <img width="2032" alt="async-timing" 
src="https://user-images.githubusercontent.com/296227/101680375-7d8c4100-3a15-11eb-8de1-8cf3e96b0557.png";>
   
   **Sync:**
   <img width="2032" alt="sync-timing" 
src="https://user-images.githubusercontent.com/296227/101680390-82e98b80-3a15-11eb-92f6-aadffd0ea7e2.png";>
   
   I'm curious what your celery concurrency is set to @ktmud @villebro. These 
are the settings used in this test:
   ```
   celery worker --app=superset.tasks.celery_app:app --pool=threads -c 20 
--loglevel=DEBUG
   ```
   
   Keep in mind also that the ultimate goal here (with SIP-39) is to use 
websockets as the event transport, which will eliminate the delay between 
polling requests, and the authentication overhead therein. The plan was to 
include short polling as a fallback if running a websocket server is not an 
option for whatever reason. As an MVP, it made sense to leverage polling to 
test the full e2e solution without introducing additional dependencies.
   
   To the point about returning cached data with the initial request, I'll look 
into the refactoring work necessary to accomplish this (as the caching logic 
currently lives in the `get_payload` methods which execute the queries), but 
architecturally, I think there's value in having consistent return values from 
the APIs regardless of the cache status. I suggest we revisit this once 
websockets are introduced, and we can do some additional performance testing.
   
   EDIT: the above test was performed with cached chart data


----------------------------------------------------------------
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.

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