From 528d04213e5123d89ae7979cbbb182ca9520736c Mon Sep 17 00:00:00 2001 From: Rohan Bansal Date: Mon, 5 Oct 2020 17:16:23 +0530 Subject: [PATCH 1/9] feat: clear failed jobs queue --- .../page/background_jobs/background_jobs.js | 22 +++++++++--- .../page/background_jobs/background_jobs.py | 34 ++++++++++++------- .../background_jobs_outer.html | 5 ++- 3 files changed, 43 insertions(+), 18 deletions(-) diff --git a/frappe/core/page/background_jobs/background_jobs.js b/frappe/core/page/background_jobs/background_jobs.js index bbc8bf049b..9dd2c10b05 100644 --- a/frappe/core/page/background_jobs/background_jobs.js +++ b/frappe/core/page/background_jobs/background_jobs.js @@ -1,5 +1,5 @@ frappe.pages['background_jobs'].on_page_load = function(wrapper) { - var page = frappe.ui.make_app_page({ + let page = frappe.ui.make_app_page({ parent: wrapper, title: __('Background Jobs'), single_column: true @@ -9,6 +9,9 @@ frappe.pages['background_jobs'].on_page_load = function(wrapper) { page.content = $(page.body).find('.table-area'); frappe.pages.background_jobs.page = page; + + let $remove_failed_btn = page.body.find('.remove-failed'); + $remove_failed_btn.click(remove_failed_jobs); } frappe.pages['background_jobs'].on_page_show = function(wrapper) { @@ -22,10 +25,10 @@ frappe.pages['background_jobs'].on_page_show = function(wrapper) { } frappe.pages.background_jobs.refresh_jobs = function() { - var page = frappe.pages.background_jobs.page; + let page = frappe.pages.background_jobs.page; // don't call if already waiting for a response - if(page.called) return; + if (page.called) return; page.called = true; frappe.call({ method: 'frappe.core.page.background_jobs.background_jobs.get_info', @@ -35,11 +38,20 @@ frappe.pages.background_jobs.refresh_jobs = function() { callback: function(r) { page.called = false; page.body.find('.list-jobs').remove(); - $(frappe.render_template('background_jobs', {jobs:r.message || []})).appendTo(page.content); + $(frappe.render_template('background_jobs', { jobs: r.message || [] })).appendTo(page.content); - if(frappe.get_route()[0]==='background_jobs') { + if (frappe.get_route()[0] === 'background_jobs') { frappe.background_jobs_timeout = setTimeout(frappe.pages.background_jobs.refresh_jobs, 2000); } } }); } + +const remove_failed_jobs = function() { + frappe.call({ + method: 'frappe.core.page.background_jobs.background_jobs.remove_failed_jobs', + callback: function (r) { + frappe.pages.background_jobs.refresh_jobs(); + } + }); +} diff --git a/frappe/core/page/background_jobs/background_jobs.py b/frappe/core/page/background_jobs/background_jobs.py index 4a94de4ace..2185149024 100644 --- a/frappe/core/page/background_jobs/background_jobs.py +++ b/frappe/core/page/background_jobs/background_jobs.py @@ -1,14 +1,13 @@ # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors # MIT License. See license.txt -from __future__ import unicode_literals -import frappe - from rq import Queue, Worker -from frappe.utils.background_jobs import get_redis_conn -from frappe.utils import format_datetime, cint, convert_utc_to_user_timezone -from frappe.utils.scheduler import is_scheduler_inactive + +import frappe from frappe import _ +from frappe.utils import cint, convert_utc_to_user_timezone, format_datetime +from frappe.utils.background_jobs import get_redis_conn +from frappe.utils.scheduler import is_scheduler_inactive colors = { 'queued': 'orange', @@ -17,6 +16,7 @@ colors = { 'finished': 'green' } + @frappe.whitelist() def get_info(show_failed=False): conn = get_redis_conn() @@ -25,11 +25,9 @@ def get_info(show_failed=False): jobs = [] def add_job(j, name): - if j.kwargs.get('site')==frappe.local.site: + if j.kwargs.get('site') == frappe.local.site: jobs.append({ - 'job_name': j.kwargs.get('kwargs', {}).get('playbook_method') \ - or j.kwargs.get('kwargs', {}).get('job_type') \ - or str(j.kwargs.get('job_name')), + 'job_name': j.kwargs.get('kwargs', {}).get('playbook_method') or j.kwargs.get('kwargs', {}).get('job_type') or str(j.kwargs.get('job_name')), 'status': j.get_status(), 'queue': name, 'creation': format_datetime(convert_utc_to_user_timezone(j.created_at)), 'color': colors[j.get_status()] @@ -44,15 +42,27 @@ def get_info(show_failed=False): for q in queues: if q.name != 'failed': - for j in q.get_jobs(): add_job(j, q.name) + for j in q.get_jobs(): + add_job(j, q.name) if cint(show_failed): for q in queues: if q.name == 'failed': - for j in q.get_jobs()[:10]: add_job(j, q.name) + for j in q.get_jobs()[:10]: + add_job(j, q.name) return jobs + +@frappe.whitelist() +def remove_failed_jobs(): + conn = get_redis_conn() + queues = Queue.all(conn) + for q in queues: + if q.name == 'failed': + q.empty() + + @frappe.whitelist() def get_scheduler_status(): if is_scheduler_inactive(): diff --git a/frappe/core/page/background_jobs/background_jobs_outer.html b/frappe/core/page/background_jobs/background_jobs_outer.html index 4da4498304..8c79806005 100644 --- a/frappe/core/page/background_jobs/background_jobs_outer.html +++ b/frappe/core/page/background_jobs/background_jobs_outer.html @@ -2,9 +2,12 @@

+
+ +

From d6775a03c030daa62cb11a226d206b16f0e7aef5 Mon Sep 17 00:00:00 2001 From: Rohan Bansal Date: Fri, 6 Nov 2020 17:10:04 +0530 Subject: [PATCH 2/9] fix: use inbuilt RQ functions to clear job --- .../page/background_jobs/background_jobs.js | 4 +- .../page/background_jobs/background_jobs.py | 74 +++++++++++-------- 2 files changed, 47 insertions(+), 31 deletions(-) diff --git a/frappe/core/page/background_jobs/background_jobs.js b/frappe/core/page/background_jobs/background_jobs.js index 9dd2c10b05..75d97b9411 100644 --- a/frappe/core/page/background_jobs/background_jobs.js +++ b/frappe/core/page/background_jobs/background_jobs.js @@ -50,8 +50,8 @@ frappe.pages.background_jobs.refresh_jobs = function() { const remove_failed_jobs = function() { frappe.call({ method: 'frappe.core.page.background_jobs.background_jobs.remove_failed_jobs', - callback: function (r) { + callback: function () { frappe.pages.background_jobs.refresh_jobs(); } }); -} +}; diff --git a/frappe/core/page/background_jobs/background_jobs.py b/frappe/core/page/background_jobs/background_jobs.py index 2185149024..a8e6c91212 100644 --- a/frappe/core/page/background_jobs/background_jobs.py +++ b/frappe/core/page/background_jobs/background_jobs.py @@ -1,6 +1,7 @@ # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors # MIT License. See license.txt +from typing import TYPE_CHECKING, Dict, List from rq import Queue, Worker import frappe @@ -9,7 +10,10 @@ from frappe.utils import cint, convert_utc_to_user_timezone, format_datetime from frappe.utils.background_jobs import get_redis_conn from frappe.utils.scheduler import is_scheduler_inactive -colors = { +if TYPE_CHECKING: + from rq.job import Job + +COLORS = { 'queued': 'orange', 'failed': 'red', 'started': 'blue', @@ -18,53 +22,65 @@ colors = { @frappe.whitelist() -def get_info(show_failed=False): +def get_info(show_failed: bool = False) -> List[Dict]: conn = get_redis_conn() queues = Queue.all(conn) workers = Worker.all(conn) jobs = [] - def add_job(j, name): - if j.kwargs.get('site') == frappe.local.site: - jobs.append({ - 'job_name': j.kwargs.get('kwargs', {}).get('playbook_method') or j.kwargs.get('kwargs', {}).get('job_type') or str(j.kwargs.get('job_name')), - 'status': j.get_status(), 'queue': name, - 'creation': format_datetime(convert_utc_to_user_timezone(j.created_at)), - 'color': colors[j.get_status()] - }) - if j.exc_info: - jobs[-1]['exc_info'] = j.exc_info + def add_job(job: Job, name: str) -> None: + if job.kwargs.get('site') == frappe.local.site: + job_info = { + 'job_name': job.kwargs.get('kwargs', {}).get('playbook_method') + or job.kwargs.get('kwargs', {}).get('job_type') + or str(job.kwargs.get('job_name')), + 'status': job.get_status(), + 'queue': name, + 'creation': format_datetime(convert_utc_to_user_timezone(job.created_at)), + 'color': COLORS[job.get_status()] + } - for w in workers: - j = w.get_current_job() - if j: - add_job(j, w.name) + if job.exc_info: + job_info['exc_info'] = job.exc_info - for q in queues: - if q.name != 'failed': - for j in q.get_jobs(): - add_job(j, q.name) + jobs.append(job_info) + # show worker jobs + for worker in workers: + job = worker.get_current_job() + if job: + add_job(job, worker.name) + + # show active queued jobs + for queue in queues: + if queue.name != 'failed': + for job in queue.jobs: + add_job(job, queue.name) + + # show failed jobs, if requested if cint(show_failed): - for q in queues: - if q.name == 'failed': - for j in q.get_jobs()[:10]: - add_job(j, q.name) + for queue in queues: + fail_registry = queue.failed_job_registry + for job_id in fail_registry.get_job_ids(): + job = queue.fetch_job(job_id) + add_job(job, queue.name) return jobs @frappe.whitelist() -def remove_failed_jobs(): +def remove_failed_jobs() -> None: conn = get_redis_conn() queues = Queue.all(conn) - for q in queues: - if q.name == 'failed': - q.empty() + for queue in queues: + fail_registry = queue.failed_job_registry + for job_id in fail_registry.get_job_ids(): + job = queue.fetch_job(job_id) + fail_registry.remove(job, delete_job=True) @frappe.whitelist() -def get_scheduler_status(): +def get_scheduler_status() -> None: if is_scheduler_inactive(): return [_("Inactive"), "red"] return [_("Active"), "green"] From 8cfd48449262caa19b0795d4b6c1c537372d0782 Mon Sep 17 00:00:00 2001 From: Rohan Bansal Date: Fri, 6 Nov 2020 17:38:10 +0530 Subject: [PATCH 3/9] fix: add forward referencing --- frappe/core/page/background_jobs/background_jobs.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frappe/core/page/background_jobs/background_jobs.py b/frappe/core/page/background_jobs/background_jobs.py index a8e6c91212..baad40009b 100644 --- a/frappe/core/page/background_jobs/background_jobs.py +++ b/frappe/core/page/background_jobs/background_jobs.py @@ -22,13 +22,13 @@ COLORS = { @frappe.whitelist() -def get_info(show_failed: bool = False) -> List[Dict]: +def get_info(show_failed=False) -> List[Dict]: conn = get_redis_conn() queues = Queue.all(conn) workers = Worker.all(conn) jobs = [] - def add_job(job: Job, name: str) -> None: + def add_job(job: 'Job', name: str) -> None: if job.kwargs.get('site') == frappe.local.site: job_info = { 'job_name': job.kwargs.get('kwargs', {}).get('playbook_method') @@ -69,7 +69,7 @@ def get_info(show_failed: bool = False) -> List[Dict]: @frappe.whitelist() -def remove_failed_jobs() -> None: +def remove_failed_jobs(): conn = get_redis_conn() queues = Queue.all(conn) for queue in queues: @@ -80,7 +80,7 @@ def remove_failed_jobs() -> None: @frappe.whitelist() -def get_scheduler_status() -> None: +def get_scheduler_status(): if is_scheduler_inactive(): return [_("Inactive"), "red"] return [_("Active"), "green"] From 921fdffa72e0a5af6f3c0246c8a061be314ca0e2 Mon Sep 17 00:00:00 2001 From: Rohan Bansal Date: Tue, 29 Dec 2020 16:11:03 +0530 Subject: [PATCH 4/9] fix: show remove button conditionally --- frappe/core/page/background_jobs/background_jobs.js | 7 +++++++ .../core/page/background_jobs/background_jobs_outer.html | 6 ++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/frappe/core/page/background_jobs/background_jobs.js b/frappe/core/page/background_jobs/background_jobs.js index 75d97b9411..397f097472 100644 --- a/frappe/core/page/background_jobs/background_jobs.js +++ b/frappe/core/page/background_jobs/background_jobs.js @@ -40,6 +40,13 @@ frappe.pages.background_jobs.refresh_jobs = function() { page.body.find('.list-jobs').remove(); $(frappe.render_template('background_jobs', { jobs: r.message || [] })).appendTo(page.content); + let $remove_failed_btn = page.body.find('.remove-failed'); + if (r.message && r.message.length > 0) { + $remove_failed_btn.show(); + } else { + $remove_failed_btn.hide(); + } + if (frappe.get_route()[0] === 'background_jobs') { frappe.background_jobs_timeout = setTimeout(frappe.pages.background_jobs.refresh_jobs, 2000); } diff --git a/frappe/core/page/background_jobs/background_jobs_outer.html b/frappe/core/page/background_jobs/background_jobs_outer.html index 8c79806005..f57339735a 100644 --- a/frappe/core/page/background_jobs/background_jobs_outer.html +++ b/frappe/core/page/background_jobs/background_jobs_outer.html @@ -6,10 +6,8 @@
- +

-
- -
+
\ No newline at end of file From e439c02561ab0fe073bd1587fb01fc88a6af22df Mon Sep 17 00:00:00 2001 From: Rohan Bansal Date: Wed, 3 Feb 2021 15:22:47 +0530 Subject: [PATCH 5/9] fix: move button to top of the page --- .../page/background_jobs/background_jobs.js | 18 ++++++++++-------- .../page/background_jobs/background_jobs.py | 6 ++++-- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/frappe/core/page/background_jobs/background_jobs.js b/frappe/core/page/background_jobs/background_jobs.js index bcd938459d..920ee2155e 100644 --- a/frappe/core/page/background_jobs/background_jobs.js +++ b/frappe/core/page/background_jobs/background_jobs.js @@ -28,6 +28,16 @@ class BackgroundJobs { } }); + // add a "Remove Failed Jobs button" + this.remove_failed_button = this.page.add_inner_button(__("Remove Failed Jobs"), () => { + frappe.call({ + method: 'frappe.core.page.background_jobs.background_jobs.remove_failed_jobs', + callback: res => { + this.refresh_jobs(); + } + }) + }); + $(frappe.render_template('background_jobs_outer')).appendTo(this.page.body); this.content = $(this.page.body).find('.table-area'); } @@ -56,14 +66,6 @@ class BackgroundJobs { this.page.body.find('.list-jobs').remove(); $(frappe.render_template('background_jobs', { jobs: res.message || [] })).appendTo(this.content); - // add a "Remove Failed Jobs button" - let $remove_failed_btn = this.body.find('.remove-failed'); - if (res.message && res.message.length > 0) { - $remove_failed_btn.show(); - } else { - $remove_failed_btn.hide(); - } - if (frappe.get_route()[0] === 'background_jobs') { setTimeout(() => this.refresh_jobs(), 2000); } diff --git a/frappe/core/page/background_jobs/background_jobs.py b/frappe/core/page/background_jobs/background_jobs.py index baad40009b..92cc995eb4 100644 --- a/frappe/core/page/background_jobs/background_jobs.py +++ b/frappe/core/page/background_jobs/background_jobs.py @@ -1,12 +1,14 @@ # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors # MIT License. See license.txt +import json from typing import TYPE_CHECKING, Dict, List + from rq import Queue, Worker import frappe from frappe import _ -from frappe.utils import cint, convert_utc_to_user_timezone, format_datetime +from frappe.utils import convert_utc_to_user_timezone, format_datetime from frappe.utils.background_jobs import get_redis_conn from frappe.utils.scheduler import is_scheduler_inactive @@ -58,7 +60,7 @@ def get_info(show_failed=False) -> List[Dict]: add_job(job, queue.name) # show failed jobs, if requested - if cint(show_failed): + if json.loads(show_failed): for queue in queues: fail_registry = queue.failed_job_registry for job_id in fail_registry.get_job_ids(): From be1c68c07dd9b592fe2cce2abaf33dbc3cd3687d Mon Sep 17 00:00:00 2001 From: Rohan Bansal Date: Wed, 3 Feb 2021 15:32:37 +0530 Subject: [PATCH 6/9] fix: sider --- frappe/core/page/background_jobs/background_jobs.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/core/page/background_jobs/background_jobs.js b/frappe/core/page/background_jobs/background_jobs.js index 920ee2155e..0b4d6792dc 100644 --- a/frappe/core/page/background_jobs/background_jobs.js +++ b/frappe/core/page/background_jobs/background_jobs.js @@ -32,10 +32,10 @@ class BackgroundJobs { this.remove_failed_button = this.page.add_inner_button(__("Remove Failed Jobs"), () => { frappe.call({ method: 'frappe.core.page.background_jobs.background_jobs.remove_failed_jobs', - callback: res => { + callback: () => { this.refresh_jobs(); } - }) + }); }); $(frappe.render_template('background_jobs_outer')).appendTo(this.page.body); From c6d61123b5d50886b9614bf1b43a96eaa6c032b7 Mon Sep 17 00:00:00 2001 From: Rohan Bansal Date: Thu, 4 Feb 2021 14:26:07 +0530 Subject: [PATCH 7/9] fix: type-check if failed jobs are to be shown --- .../core/page/background_jobs/background_jobs.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/frappe/core/page/background_jobs/background_jobs.py b/frappe/core/page/background_jobs/background_jobs.py index 92cc995eb4..847b23bd3e 100644 --- a/frappe/core/page/background_jobs/background_jobs.py +++ b/frappe/core/page/background_jobs/background_jobs.py @@ -15,7 +15,7 @@ from frappe.utils.scheduler import is_scheduler_inactive if TYPE_CHECKING: from rq.job import Job -COLORS = { +JOB_COLORS = { 'queued': 'orange', 'failed': 'red', 'started': 'blue', @@ -25,6 +25,9 @@ COLORS = { @frappe.whitelist() def get_info(show_failed=False) -> List[Dict]: + if isinstance(show_failed, str): + show_failed = json.loads(show_failed) + conn = get_redis_conn() queues = Queue.all(conn) workers = Worker.all(conn) @@ -39,7 +42,7 @@ def get_info(show_failed=False) -> List[Dict]: 'status': job.get_status(), 'queue': name, 'creation': format_datetime(convert_utc_to_user_timezone(job.created_at)), - 'color': COLORS[job.get_status()] + 'color': JOB_COLORS[job.get_status()] } if job.exc_info: @@ -53,15 +56,14 @@ def get_info(show_failed=False) -> List[Dict]: if job: add_job(job, worker.name) - # show active queued jobs for queue in queues: + # show active queued jobs if queue.name != 'failed': for job in queue.jobs: add_job(job, queue.name) - # show failed jobs, if requested - if json.loads(show_failed): - for queue in queues: + # show failed jobs, if requested + if show_failed: fail_registry = queue.failed_job_registry for job_id in fail_registry.get_job_ids(): job = queue.fetch_job(job_id) From 446f5785a5db301de68adffe9e114b3ebafe0b6f Mon Sep 17 00:00:00 2001 From: Rohan Bansal Date: Mon, 15 Feb 2021 15:06:14 +0530 Subject: [PATCH 8/9] test: add tests for removing failed jobs --- frappe/tests/test_background_jobs.py | 31 ++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 frappe/tests/test_background_jobs.py diff --git a/frappe/tests/test_background_jobs.py b/frappe/tests/test_background_jobs.py new file mode 100644 index 0000000000..5b66183333 --- /dev/null +++ b/frappe/tests/test_background_jobs.py @@ -0,0 +1,31 @@ +import unittest + +from rq import Queue + +import frappe +from frappe.core.page.background_jobs.background_jobs import remove_failed_jobs +from frappe.utils.background_jobs import get_redis_conn + + +class TestBackgroundJobs(unittest.TestCase): + def test_remove_failed_jobs(self): + frappe.enqueue(method="frappe.tests.test_background_jobs.fail_function") + + conn = get_redis_conn() + queues = Queue.all(conn) + + for queue in queues: + if queue.name == "default": + fail_registry = queue.failed_job_registry + self.assertGreater(fail_registry.count, 0) + + remove_failed_jobs() + + for queue in queues: + if queue.name == "default": + fail_registry = queue.failed_job_registry + self.assertEqual(fail_registry.count, 0) + + +def fail_function(): + return 1 / 0 From 7c05fd2a177eeb4e647a6ebc6eb6229fdc27bcd5 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Wed, 10 Mar 2021 10:29:07 +0530 Subject: [PATCH 9/9] test: Wait for enqueued job to execute --- frappe/tests/test_background_jobs.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/frappe/tests/test_background_jobs.py b/frappe/tests/test_background_jobs.py index 5b66183333..88783f14f1 100644 --- a/frappe/tests/test_background_jobs.py +++ b/frappe/tests/test_background_jobs.py @@ -5,24 +5,26 @@ from rq import Queue import frappe from frappe.core.page.background_jobs.background_jobs import remove_failed_jobs from frappe.utils.background_jobs import get_redis_conn +import time class TestBackgroundJobs(unittest.TestCase): def test_remove_failed_jobs(self): - frappe.enqueue(method="frappe.tests.test_background_jobs.fail_function") - + frappe.enqueue(method="frappe.tests.test_background_jobs.fail_function", queue="short") + # wait for enqueued job to execute + time.sleep(2) conn = get_redis_conn() queues = Queue.all(conn) for queue in queues: - if queue.name == "default": + if queue.name == "short": fail_registry = queue.failed_job_registry self.assertGreater(fail_registry.count, 0) remove_failed_jobs() for queue in queues: - if queue.name == "default": + if queue.name == "short": fail_registry = queue.failed_job_registry self.assertEqual(fail_registry.count, 0)