Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: bump client side chart timeouts to use the SUPERSET_WEBSERVER_TIMEOUT #28018

Merged
merged 2 commits into from
Apr 15, 2024
Merged

fix: bump client side chart timeouts to use the SUPERSET_WEBSERVER_TIMEOUT #28018

merged 2 commits into from
Apr 15, 2024

Conversation

eschutho
Copy link
Member

SUMMARY

We're seeing a lot of client side timeouts in annotation layers and for the chart/data endpoints. I'm not sure why we had the timeout set to only 1 minute, since the chart/data endpoints are traditionally some of the longest running queries.
Screenshot 2024-04-12 at 1 58 22 PM

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

@eschutho eschutho force-pushed the elizabeth/bump-client-timeout branch from dd4845e to 4a2f84b Compare April 12, 2024 21:00
eschutho
eschutho commented Apr 12, 2024
View reviewed changes
superset-frontend/src/components/Chart/chartAction.js Outdated
@@ -248,7 +248,7 @@ export async function getChartDataRequest({

export function runAnnotationQuery({
annotation,
timeout = 60,
timeout = 300,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is for us to make this timeout configurable in config.py like we do with the server timeouts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on putting this in config such that we can see timeouts in one place and if someone wants to configure it for their instance they just have to update in config.py vs having to find all places and update it there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, would be nice to make this a config :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kk, I'll make that change then. 👍

Copy link
Member

@rusackas rusackas Apr 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be sensible to use one of the existing timeout configs rather than adding a new one?
N/M, you did!

@eschutho eschutho marked this pull request as ready for review April 12, 2024 21:27
@eschutho eschutho force-pushed the elizabeth/bump-client-timeout branch from 4aa4ebd to 7d262c8 Compare April 12, 2024 23:27
@pull-request-size pull-request-size bot added size/L and removed size/S labels Apr 12, 2024
eschutho
eschutho commented Apr 12, 2024
View reviewed changes
superset-frontend/src/components/Chart/chartAction.js
const sliceKey = key || Object.keys(getState().charts)[0];
const {charts, common} = getState()
const sliceKey = key || Object.keys(charts)[0];
const queryTimeout = timeout || common.conf.SUPERSET_WEBSERVER_TIMEOUT;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This config was already used elsewhere, so I decided to reuse it instead of creating a new one. Maybe in a follow up PR if we feel want the webserver and client timeouts to be different, we can update all of them. But for now, it's consistent with other parts of the js codebase.

@eschutho eschutho force-pushed the elizabeth/bump-client-timeout branch from 7d262c8 to 60125d7 Compare April 12, 2024 23:35
@eschutho eschutho changed the title fix: bump client side chart timeouts to 5 minutes fix: bump client side chart timeouts to use the SUPERSET_WEBSERVER_TIMEOUT Apr 13, 2024
@eschutho eschutho merged commit 99c414e into master Apr 15, 2024
31 checks passed
@rusackas rusackas deleted the elizabeth/bump-client-timeout branch April 15, 2024 20:21
@michael-s-molina michael-s-molina added v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch v3.1 Label added by the release manager to track PRs to be included in the 3.1 branch labels Apr 16, 2024
michael-s-molina pushed a commit that referenced this pull request Apr 16, 2024
@eschutho @michael-s-molina
fix: bump client side chart timeouts to use the SUPERSET_WEBSERVER_TI…
964dc61 
…MEOUT (#28018)

(cherry picked from commit 99c414e)
michael-s-molina pushed a commit that referenced this pull request Apr 16, 2024
@eschutho @michael-s-molina
fix: bump client side chart timeouts to use the SUPERSET_WEBSERVER_TI…
61d6b9c 
…MEOUT (#28018)

(cherry picked from commit 99c414e)
michael-s-molina pushed a commit that referenced this pull request Apr 16, 2024
@eschutho @michael-s-molina
fix: bump client side chart timeouts to use the SUPERSET_WEBSERVER_TI…
5f09d95 
…MEOUT (#28018)

(cherry picked from commit 99c414e)
michael-s-molina pushed a commit that referenced this pull request Apr 16, 2024
@eschutho @michael-s-molina
fix: bump client side chart timeouts to use the SUPERSET_WEBSERVER_TI…
aa75891 
…MEOUT (#28018)

(cherry picked from commit 99c414e)
betodealmeida pushed a commit that referenced this pull request Apr 25, 2024
@eschutho @sadpandajoe
fix: bump client side chart timeouts to use the SUPERSET_WEBSERVER_TI…
fc8fa90 
…MEOUT (#28018)

(cherry picked from commit 99c414e)
qleroy pushed a commit to qleroy/superset that referenced this pull request Apr 28, 2024
@eschutho @qleroy
fix: bump client side chart timeouts to use the SUPERSET_WEBSERVER_TI…
57f022c 
…MEOUT (apache#28018)
jzhao62 pushed a commit to jzhao62/superset that referenced this pull request May 16, 2024
@eschutho @jzhao62
fix: bump client side chart timeouts to use the SUPERSET_WEBSERVER_TI…
3e9ed3e 
…MEOUT (apache#28018)
@mistercrunch mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rusackas rusackas rusackas left review comments

@craig-rueda craig-rueda craig-rueda left review comments

@sadpandajoe sadpandajoe sadpandajoe left review comments

@dpgaspar dpgaspar dpgaspar approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/L v3.1 Label added by the release manager to track PRs to be included in the 3.1 branch v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch 🍒 3.1.3 🍒 4.0.1 🍒 4.0.2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@eschutho @rusackas @craig-rueda @dpgaspar @sadpandajoe @mistercrunch @michael-s-molina