refresh_queries shouldn't break because of a single query having a bad schedule object

[rauchy]

Issue Summary

We recently had an issue where a single query had a bad schedule object:

{
    "interval": 604800, 
    "until": null,
    "day_of_week": "Sunday", 
    "time": "Invalid date"
}

And it caused Query.outdated_queries to blow up, which in turn stopped refresh_queries from working.

We should make sure there is enough exception handling that a single query won't stop the scheduler from working.

Technical details:

• Redash Version: 8

[rauchy]

While we should basically move on to the next query when an error occurs (and not stop the the loop), we should also handle a case where jobs already expire and can't be fetched (thus raising an exception), and remove the lock in these cases, so these queries can be executed as part of this issue.

[rauchy]

While wrapping it in try/except blocks, I felt that refresh_queries has too much going on and wasn't easy to read, so I've taken the opportunity to refactor it into something (I see as) more readable.

[arikfr]

Wasn't the Python version of this a bit easier to understand?

[rauchy]

Yes, it was. I just found the fact that the filtering logic is divided between the query and the enqueuing loops more confusing. I prefer a (slightly) more complex query over mixed concerns.

[arikfr]

But it's split anyway, because later we do tests on the schedule data to see if a query is outdated. How's checking if interval is None any different from the other tests?

[arikfr]

Btw, I'm not sure if this test is even needed. I believe it's from an interim version of the code where there was always a schedule object, but later we switched to using None to signal no schedule.

[rauchy]

How's checking if interval is None any different from the other tests?

The only difference is how much complexity it adds. Checking for until was way harder to understand in the query.

later we switched to using None to signal no schedule

Yeah, looks like there are no residues of that so it's safe to remove. (6ad764f)

[arikfr]

1. This should go into Sentry.
2. Considering the way our scheduler works this is going to be spammy. Maybe we should report this and then unschedule?

[rauchy]

Regarding #2 - this function should be idempotent. It's kinda strange to call outdated_queries and have your schedule change as a result.

I feel more comfortable with calling track_failure in these cases - this way users could find out that there was an error with running the query, and it would increase the schedule_failures counter, which would push the next reschedule down the exponential backoff track.

[rauchy]

Actually it would make more sense to use track_failure in _enqueue_queries instead.

[arikfr]

I don't think schedule_failures will work here, because it won't reach the part of the code that calculates the next iteration.

While usually you're right in the approach, we're talking about queries with malformed schedule object. Also, sending a user a notification about it doesn't feel useful as in most cases they won't know what to do about it.

Another approach we can take is to introduce a disabled boolean to the schedule object which we will trigger in such cases (and later can review these). The downside here is that it won't work in case it's completely malformed (not even a valid JSON).

[rauchy]

well, disabled could be set to True if schedule is broken or schedule['disabled'] is True.

[arikfr]

👍

[rauchy]

Reporting to Sentry is done in 57b1d4f.

[arikfr]

While it's not very likely, it still can happen that between the call to exists to fetch, the job will be removed. Is there a version of fetch that returns None if job doesn't exist? Or maybe just catch exception?

[rauchy]

24ca387

[arikfr]

Wouldn't it be easier to understand, more performant (less loops) and simpler if it was something like:

for query in models.Query.outdated_queries():
    # logic from _skip_unrefreshable_queries
    if not should_refresh_query(query):
        continue
    try:
        enqueue_query(
            # logic from _apply_default_parameters
            apply_default_parameters(query.query_text),
            query.data_source,
            query.user_id,
            scheduled_query=query,
            metadata={"Query ID": query.id, "Username": "Scheduled"},
      )
    except Exception as e:
        logging.info("Could not enqueue query %d due to %s", query.id, repr(e))

[arikfr]

Also, similar comment about exception logging as with the one in outdated_queries. Except that here we should only report to Sentry, but not unschedule.

[rauchy]

Simplicity / understandability is a matter of preference. Personally I thought of the journey from outdated_queries to enqueued as a set of transformations and filters, so I took a FP-based approach. I've switched to your suggestion in ef2eb39.

Regarding more performant - not sure, those were generator loops and I think there's a built-in optimization there.

[arikfr]

Looks good. We might need to tweak the Sentry reporting, but let's see how it goes.