Fix issue with scheduled queries #7111

ezraodio1 · 2024-08-06T13:55:01Z

What type of PR is this?

Description

There's currently an issue in Redash where some queries fail to refresh on their schedule. This happens when the query has no latest_query_data, which makes retrieved_at = None, which makes it so the query never gets added to outdated_queries. This PR fixes that.

The problem can be reproduced by running the unit test that's part of this PR (without making the fix first, of course)

def test_enqueues_scheduled_query_without_latest_query_data(self):
        """
        Queries with a schedule but no latest_query_data will still be reported by Query.outdated_queries()
        """
        query = self.factory.create_query(
            schedule=self.schedule(interval="60"),
            data_source=self.factory.create_data_source(),
        )

        outdated_queries = models.Query.outdated_queries()
        self.assertEqual(query.latest_query_data, None)
        self.assertEqual(len(outdated_queries), 1)
        self.assertIn(query, outdated_queries)

How is this tested?

Unit tests (pytest, jest)
E2E Tests (Cypress)
Manually
N/A

Related Tickets & Documents

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

eradman

Tested this manually, seems to check out

justinclift · 2024-09-20T16:30:23Z

@arikfr Any idea if this will interact with the scheduled queries hash issue you were looking at yesterday?

arikfr · 2024-09-21T04:20:09Z

redash/models/__init__.py

@@ -389,6 +389,8 @@ def groups(self):
 def should_schedule_next(previous_iteration, now, interval, time=None, day_of_week=None, failures=0):
    # if time exists then interval > 23 hours (82800s)
    # if day_of_week exists then interval > 6 days (518400s)
+    if previous_iteration is None:
+        return False


Why not return True here instead of the or not retrieved_at in the calling function?

I don't remember there being a specific reason. I think both ways are logically equivalent.

While it's true that both will have the same result, it does make the code more confusing: should_schedule_next returns False, but we override it.. it will be more readable to have it return True which is what we want in this case anyway.

arikfr · 2024-09-21T04:20:17Z

redash/models/__init__.py

+                if all(value is None for value in query.schedule.values()):
+                    continue


What's this for?

It's been a little while since I looked at this, but I think this is the answer:

The only way to check whether a query doesn't have a refresh schedule is to ensure that everything in query.schedule.values() is None. For example, interval, until, time are all keys in the query.schedule. It's possible to have a query refresh schedule when just one of these keys is not None.

If a query doesn't have a refresh schedule, then we can just continue to the next query in the loop.

When a query does not have a schedule then the schedule value is null (or empty). It's possible there is some edge case scenario that might keep schedule object with empty values, but I would assume it's not common?

To keep the code maintainable I would rather avoid this change in this PR and if you still feel it's necessary let's bring it back in a separate one and have the discussion there (and potentially add tests for this case).

I think this is worth double checking. I could be mistaken, but I remember this edge case popping up during my testing. Perhaps if a query had a schedule and then that schedule was later removed?

Sounds like we'll need to create a follow up PR to this one if/when we merge this. 😄

arikfr

One last review. Please let us know if you have the bandwidth to apply the changes. Thanks!

arikfr · 2024-10-07T09:30:55Z

redash/models/__init__.py

+                if all(value is None for value in query.schedule.values()):
+                    continue


When a query does not have a schedule then the schedule value is null (or empty). It's possible there is some edge case scenario that might keep schedule object with empty values, but I would assume it's not common?

To keep the code maintainable I would rather avoid this change in this PR and if you still feel it's necessary let's bring it back in a separate one and have the discussion there (and potentially add tests for this case).

arikfr · 2024-10-07T09:32:39Z

redash/models/__init__.py

@@ -389,6 +389,8 @@ def groups(self):
 def should_schedule_next(previous_iteration, now, interval, time=None, day_of_week=None, failures=0):
    # if time exists then interval > 23 hours (82800s)
    # if day_of_week exists then interval > 6 days (518400s)
+    if previous_iteration is None:
+        return False


While it's true that both will have the same result, it does make the code more confusing: should_schedule_next returns False, but we override it.. it will be more readable to have it return True which is what we want in this case anyway.

ezraodio1 · 2024-10-09T00:26:15Z

I lost access to the VM that I was using over the summer for Redash dev work unfortunately, so I think it's easiest if someone else applies the changes.

justinclift · 2024-10-09T01:03:06Z

@ezraodio1 If you need a new VM for doing Redash dev work on, then I'm happy to spin one up for you. Would that be useful? 😄

ezraodio1 · 2024-10-15T20:44:45Z

I'm taking a tough course load right, so I'm pretty busy. I'd definitely be interested after this semester ends, though. I'll try to spin up a new VM for myself.

justinclift · 2024-10-15T21:21:56Z

No worries. Keep it in mind as an option if/when it'd be useful.

Good luck with your course load, hopefully it all goes really well. 😄

eradman approved these changes Sep 20, 2024

View reviewed changes

Add scheduled queries with no latest_query_data to outdated_queries

f1b6be0

eradman force-pushed the fix-query-schedule branch from 0d863b8 to f1b6be0 Compare September 20, 2024 14:24

arikfr reviewed Sep 21, 2024

View reviewed changes

arikfr mentioned this pull request Oct 7, 2024

Fixes #6767: correctly rehash queries in a migration #7184

Open

2 tasks

arikfr reviewed Oct 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with scheduled queries #7111

Fix issue with scheduled queries #7111

ezraodio1 commented Aug 6, 2024 •

edited

Loading

eradman left a comment

justinclift commented Sep 20, 2024

arikfr Sep 21, 2024

ezraodio1 Sep 21, 2024

arikfr Oct 7, 2024

arikfr Sep 21, 2024

ezraodio1 Sep 21, 2024

arikfr Oct 7, 2024

ezraodio1 Oct 9, 2024

justinclift Oct 18, 2024

arikfr left a comment

arikfr Oct 7, 2024

arikfr Oct 7, 2024

ezraodio1 commented Oct 9, 2024

justinclift commented Oct 9, 2024 •

edited

Loading

ezraodio1 commented Oct 15, 2024

justinclift commented Oct 15, 2024

		if all(value is None for value in query.schedule.values()):
		continue

Fix issue with scheduled queries #7111

Are you sure you want to change the base?

Fix issue with scheduled queries #7111

Conversation

ezraodio1 commented Aug 6, 2024 • edited Loading

What type of PR is this?

Description

How is this tested?

Related Tickets & Documents

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

eradman left a comment

Choose a reason for hiding this comment

justinclift commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arikfr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezraodio1 commented Oct 9, 2024

justinclift commented Oct 9, 2024 • edited Loading

ezraodio1 commented Oct 15, 2024

justinclift commented Oct 15, 2024

ezraodio1 commented Aug 6, 2024 •

edited

Loading

justinclift commented Oct 9, 2024 •

edited

Loading