-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Set default query execution time limit to unlimited #4626
Conversation
redash/settings/__init__.py
Outdated
@@ -222,6 +222,14 @@ | |||
os.environ.get("REDASH_STATIC_ASSETS_PATH", "../client/dist/") | |||
) | |||
|
|||
# Time limit for scheduled queries. Set this to -1 to execute without a time limit. | |||
SCHEDULED_QUERY_TIME_LIMIT = int( | |||
os.environ.get("REDASH_SCHEDULED_QUERY_TIME_LIMIT", 3600) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I look at the existing implementation, I wonder if we should use -1
as the default to remain backward compatible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
+0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jezdez you have queries running longer than an hour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if currently, but it happened in the past. Granted, we're probably not the average user group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that an hour default value is definitely reasonable for most cases. I just wonder if we should cause issues for any existing deployments :\
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I think I change my initial response to +0, since this is a settings we can set it to a larger value before deploy. IOW don't consider our use case as a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any value in imposing a default limit. As long as it's configurable, a -1
sounds good.
What type of PR is this? (check all applicable)
Description
After migrating query executions to RQ, time limits were implicitly set by RQ to 180 seconds. This PR changes the default to 1 hour and adds some docs on how to remove time limits.