-
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
Fix issue with scheduled queries #7111
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
if time is None: | ||
ttl = int(interval) | ||
next_iteration = previous_iteration + datetime.timedelta(seconds=ttl) | ||
|
@@ -608,17 +610,23 @@ def outdated_queries(cls): | |
if schedule_until <= now: | ||
continue | ||
|
||
if all(value is None for value in query.schedule.values()): | ||
continue | ||
Comment on lines
+613
to
+614
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's this for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 If a query doesn't have a refresh schedule, then we can just continue to the next query in the loop. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like we'll need to create a follow up PR to this one if/when we merge this. 😄 |
||
|
||
retrieved_at = scheduled_queries_executions.get(query.id) or ( | ||
query.latest_query_data and query.latest_query_data.retrieved_at | ||
) | ||
|
||
if should_schedule_next( | ||
retrieved_at or now, | ||
now, | ||
query.schedule["interval"], | ||
query.schedule["time"], | ||
query.schedule["day_of_week"], | ||
query.schedule_failures, | ||
if ( | ||
should_schedule_next( | ||
retrieved_at, | ||
now, | ||
query.schedule["interval"], | ||
query.schedule["time"], | ||
query.schedule["day_of_week"], | ||
query.schedule_failures, | ||
) | ||
or not retrieved_at | ||
): | ||
key = "{}:{}".format(query.query_hash, query.data_source_id) | ||
outdated_queries[key] = query | ||
|
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.
Why not return True here instead of the
or not retrieved_at
in the calling function?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 remember there being a specific reason. I think both ways are logically equivalent.
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.
While it's true that both will have the same result, it does make the code more confusing:
should_schedule_next
returnsFalse
, but we override it.. it will be more readable to have it return True which is what we want in this case anyway.