Celery task to clear schedule was added #3801
Conversation
There was a problem hiding this comment.
Housekeeping FTW! 🧹
I personally find multiple return values a bit of a code smell here, for two reasons:
- High fan-in: while
outdated_queriesis primarily used in therefresh_queriestask, it is called multiple times in tests, so the ordinality becomes baked into several places in code. - Name bias:
outdated_queriesfetches outdated queries, as in, queries that need to be updated with new values. Housekeeping feels like a side-effect here.
I understand the motivation here is to reuse the database query in Query.outdated_queries, but for code clarity, I would consider splitting this up (if it's not costly, and I tend to believe its not), or finding an elegant solution to the two problems I stated above.
|
@rauchy that perfectly makes sense, thanks. I've made some changes, hope everything is good now once all checks are passed |
|
There are some visual changes that seem unrelated |
redash/worker.py
Outdated
| @@ -22,6 +22,10 @@ | |||
| 'task': 'redash.tasks.refresh_queries', | |||
| 'schedule': timedelta(seconds=30) | |||
| }, | |||
| 'empty_schedules': { | |||
| 'task': 'redash.tasks.empty_schedules', | |||
| 'schedule': timedelta(seconds=30) | |||
There was a problem hiding this comment.
IMO this task can run at a much lower frequency. (say, once an hour)
redash/models/__init__.py
Outdated
| .filter(Query.schedule.isnot(None)) | ||
| .order_by(Query.id) | ||
| ) | ||
| past_scheduled_queries = [] |
There was a problem hiding this comment.
Does it feel like a comprehension or filter would be more apt here? Not sure.
There was a problem hiding this comment.
Used filter here. Also, added another unit test for this method specifically
tests/tasks/test_refresh_queries.py
Outdated
| @@ -65,3 +67,11 @@ def test_enqueues_parameterized_queries(self): | |||
| add_job_mock.assert_called_with( | |||
| "select 42", query.data_source, query.user_id, | |||
| scheduled_query=query, metadata=ANY) | |||
|
|
|||
| def test_empty_schedules(self): | |||
There was a problem hiding this comment.
This guy deserves his own test class too :)
|
Some visual changes yet again :) |
Just sync with master and they'll be gone 🙂 |
|
Hi @rauchy, could you check my latest changes, please? |
| def empty_schedules(): | ||
| queries = models.Query.past_scheduled_queries() | ||
| for query in queries: | ||
| query.schedule = None |
There was a problem hiding this comment.
Let's add a log message here, just so admins will be able to traceback changes.
tests/test_models.py
Outdated
| query.latest_query_data = query_result | ||
|
|
||
| queries = models.Query.past_scheduled_queries() | ||
| self.assertIn(query, queries) |
There was a problem hiding this comment.
This feels very similar to QueryTest.test_past_scheduled_queries -- why two tests?
There was a problem hiding this comment.
I just realised that QueryTest would be more appropriate place, but forgot to delete it from here :P
|
Thanks, @aidarbek ! :) |
* Celery task to clear schedule was added * fix formating * empty_schedules task was put in separate task * worker interval changed, new tests added * past artifact deleted * test queries moved to right class, lambda was used to filter data * unnecessary changes eliminated * more unnecessary files deleted * line shortened * Line shortened more * codeclimate changes * Unused test deleted, logs added
What type of PR is this? (check all applicable)
Description
Related Tickets & Documents
Solves #3375 issue
Mobile & Desktop Screenshots/Recordings (if there are UI changes)