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.

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

Syncing the timeout param from backend #3329

Merged
merged 2 commits into from
Aug 18, 2017

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Aug 17, 2017

Before, the client-side timeout was hard coded and not aligned with the backend's setting. You would have to recompile the JS bundle with a different value if you wanted to change it.

Now the value set in superset_config.py is sent to the client and the client will timeout regardless of it gets a web response or not.

fixes #3030 (I think?)

@coveralls
Copy link

coveralls commented Aug 17, 2017

Coverage Status

Coverage remained the same at 69.397% when pulling 5abc0f690a7e02227d0e91155d56e3c53f04ced0 on mistercrunch:timeout_backend into 9f3aeb2 on apache:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.397% when pulling 5abc0f690a7e02227d0e91155d56e3c53f04ced0 on mistercrunch:timeout_backend into 9f3aeb2 on apache:master.

@coveralls
Copy link

coveralls commented Aug 18, 2017

Coverage Status

Coverage remained the same at 69.388% when pulling cbd8cfd on mistercrunch:timeout_backend into a7ba6e4 on apache:master.

@mistercrunch mistercrunch merged commit 2923a12 into apache:master Aug 18, 2017
@mistercrunch mistercrunch deleted the timeout_backend branch August 18, 2017 22:49
@xrmx
Copy link
Contributor

xrmx commented Aug 19, 2017

A couple of late comments:

  • docs/faq.rst needs update as it still references QUERY_TIMEOUT_THRESHOLD
  • why a default is needed in superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx if we are reading it from the config? And if we need it why 45 and not 60 as the backend?

@andreineculau
Copy link

yup, I was going to make the same comment as @xrmx above - why is a default needed in the first place?

@mistercrunch
Copy link
Member Author

2 - @andreineculau @xrmx it's a fixture for the unit test

@xrmx
Copy link
Contributor

xrmx commented Aug 30, 2017

@mistercrunch Ah! missed the _spec in the name :)

@maxsonic
Copy link

@mistercrunch I got the timeout in the slice panel. It seems that the default timeout in the runQuery is 60s and it is not synced with the timeout set in the superset_config.py.

image

@xrmx
Copy link
Contributor

xrmx commented Sep 11, 2017

@maxsonic that's a default, the caller should be updated

@tuziluobu
Copy link

What's the value which the client will timeout regardless of it gets a web response set in superset_config.py ?

const queryDuration = moment.duration(this.props.query.endDttm - this.props.query.startDttm);
if (Math.round(queryDuration.asMilliseconds()) > QUERY_TIMEOUT_THRESHOLD) {
Copy link

Choose a reason for hiding this comment

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

Can anyone tell which is the new constant supplanting QUERY_TIMEOUT_THRESHOLD.

Copy link
Contributor

Choose a reason for hiding this comment

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

SUPERSET_WEBSERVER_TIMEOUT

Copy link

Choose a reason for hiding this comment

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

Thanks @xrmx . why this name got changed ?

@tuziluobu
Copy link

"Query timeout - visualization query are set to timeout at 60 seconds. Perhaps your data has grown, your database is under unusual load, or you are simply querying a data source that is to large to be processed within the timeout range. If that is the case, we recommend that you summarize your data further."

please How to resolve the problem? I set SUPERSET_WEBSERVER_TIMEOUT=600 in superset_config.py,but It's not work。

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.19.1 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.19.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flawed logic in the new visualization timeout
6 participants