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

Dynamic query time limits #3702

Merged
merged 6 commits into from
Apr 15, 2019
Merged

Dynamic query time limits #3702

merged 6 commits into from
Apr 15, 2019

Conversation

rauchy
Copy link
Contributor

@rauchy rauchy commented Apr 11, 2019

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

This PR allows users to define their own custom query time limit decisions by supplying their own function to determine the limit for a adhoc/scheduled query run by a specific user.

Related Tickets & Documents

#3700

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@rauchy rauchy requested a review from arikfr April 11, 2019 10:50

# Replace this method with your own implementation in case you want to limit the time limit on certain queries or users.
def get_query_time_limit(is_scheduled, user_id):
return None if is_scheduled else int_or_none(os.environ.get('REDASH_ADHOC_QUERY_TIME_LIMIT', None))
Copy link
Member

Choose a reason for hiding this comment

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

Let's take this opportunity to introduce a value for scheduled queries as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 2885e34

from .helpers import int_or_none

# Replace this method with your own implementation in case you want to limit the time limit on certain queries or users.
def get_query_time_limit(is_scheduled, user_id):
Copy link
Member

Choose a reason for hiding this comment

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

I thought we have a User object at hand when calling this. Considering we don't, let's pass in org_id as well (or only org_id?).

Copy link
Contributor Author

@rauchy rauchy Apr 13, 2019

Choose a reason for hiding this comment

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

If the purpose of extracting this to a dynamic class was to allow users to customize this according to their needs, I'd say a user_id would allow more cases to be handled. I say we provide both. 5dc5340

@rauchy rauchy merged commit 5b30d08 into master Apr 15, 2019
@rauchy rauchy deleted the dynamic-query-time-limits branch April 15, 2019 09:06
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
* extract time limit decisions to a dynamic settings function

* introduce environment variable for scheduled query time limits

* pass in org_id to query_time_limit

* add an interaction test that verifies that time limits are applied to
jobs

* really important newlines according to CodeClimate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants