From 687b790d5274653027731ac6f0bc074f5b10e8b9 Mon Sep 17 00:00:00 2001 From: Jimmy Royer Date: Sun, 5 Nov 2023 22:56:00 -0500 Subject: [PATCH] Refactoring retry periods for sms high priority --- app/celery/provider_tasks.py | 1 - app/notifications/__init__.py | 56 ++++--------------- app/notifications/process_notifications.py | 2 - .../test_process_notification.py | 39 +++++-------- 4 files changed, 25 insertions(+), 73 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 1d3dac31e0..f284a78028 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -130,7 +130,6 @@ def _deliver_sms(self, notification_id): # retry with the default delay. current_app.logger.exception("SMS notification delivery for id: {} failed".format(notification_id)) self.retry(**build_retry_task_params(notification.notification_type, notification.template.process_type)) - # self.retry(kwargs=build_retry_task_params(notification.notification_type, notification.template.process_type)) except self.MaxRetriesExceededError: message = ( "RETRY FAILED: Max retries reached. The task send_sms_to_provider failed for notification {}. " diff --git a/app/notifications/__init__.py b/app/notifications/__init__.py index effd5c8c62..b94d9f7419 100644 --- a/app/notifications/__init__.py +++ b/app/notifications/__init__.py @@ -3,52 +3,16 @@ from flask import current_app from typing import Any, Dict -# Default retry policy for all notifications. -RETRY_POLICY_DEFAULT = { - "max_retries": 48, - "interval_start": 300, - "interval_step": 300, - "interval_max": 300, - "retry_errors": None, +# Default retry periods for sending notifications. +RETRY_DEFAULT = 300 +RETRY_HIGH = 25 + +RETRY_PERIODS = { + BULK: RETRY_DEFAULT, + NORMAL: RETRY_DEFAULT, + PRIORITY: RETRY_HIGH, } -# Retry policy for high priority notifications. -RETRY_POLICY_HIGH = { - "max_retries": 48, - "interval_start": 25, - "interval_step": 25, - "interval_max": 25, - "retry_errors": None, -} - -# Retry policies for each notification priority lanes. -RETRY_POLICIES = { - BULK: RETRY_POLICY_DEFAULT, - NORMAL: RETRY_POLICY_DEFAULT, - PRIORITY: RETRY_POLICY_HIGH, -} - - -def build_delivery_task_params(notification_type: str, notification_process_type: str) -> Dict[str, Any]: - """ - Build task params for the sending parameter tasks. - - If the notification is a high priority SMS, set the retry policy to retry every 25 seconds - else fall back to the default retry policy of retrying every 5 minutes. - """ - if current_app.config["FF_CELERY_CUSTOM_TASK_PARAMS"] is False: - return {} - - params: dict[str, Any] = {} - params["retry"] = True - # Overring the retry policy is only supported for SMS for now; - # email support coming later. - if notification_type == SMS_TYPE: - params["retry_policy"] = RETRY_POLICIES[notification_process_type] - else: - params["retry_policy"] = RETRY_POLICY_DEFAULT - return params - def build_retry_task_params(notification_type: str, notification_process_type: str) -> Dict[str, Any]: """ @@ -64,7 +28,7 @@ def build_retry_task_params(notification_type: str, notification_process_type: s # Overring the retry policy is only supported for SMS for now; # email support coming later. if notification_type == SMS_TYPE: - params["countdown"] = RETRY_POLICIES[notification_process_type]["interval_step"] + params["countdown"] = RETRY_PERIODS[notification_process_type] else: - params["countdown"] = RETRY_POLICY_DEFAULT["interval_step"] + params["countdown"] = RETRY_DEFAULT return params diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 1da7aaba53..56a2dc98d3 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -35,7 +35,6 @@ ScheduledNotification, Service, ) -from app.notifications import build_delivery_task_params from app.types import VerifiedNotification from app.utils import get_delivery_queue_for_template, get_template_instance from app.v2.errors import BadRequestError @@ -214,7 +213,6 @@ def db_save_and_send_notification(notification: Notification): deliver_task.apply_async( [str(notification.id)], queue=notification.queue_name, - **build_delivery_task_params(notification.notification_type, notification.template.process_type), ) except Exception: dao_delete_notifications_by_id(notification.id) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index f5771d806e..04f4c60d59 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -26,11 +26,7 @@ ScheduledNotification, Template, ) -from app.notifications import ( - RETRY_POLICY_DEFAULT, - RETRY_POLICY_HIGH, - build_delivery_task_params, -) +from app.notifications import RETRY_PERIODS, build_retry_task_params from app.notifications.process_notifications import ( choose_queue, create_content_for_notification, @@ -1062,9 +1058,7 @@ def test_db_save_and_send_notification_sends_to_queue( db_save_and_send_notification(notification=notification) - mocked.assert_called_once_with( - [str(notification.id)], queue=expected_queue, retry=True, retry_policy=RETRY_POLICY_DEFAULT - ) + mocked.assert_called_once_with([str(notification.id)], queue=expected_queue) def test_db_save_and_send_notification_throws_exception_deletes_notification( self, sample_template, sample_api_key, sample_job, mocker @@ -1101,25 +1095,22 @@ def test_db_save_and_send_notification_throws_exception_deletes_notification( assert NotificationHistory.query.count() == 0 @pytest.mark.parametrize( - ("notification_type, process_type, expected_retry, expected_retry_period"), + ("notification_type, process_type, expected_retry_period"), [ - (EMAIL_TYPE, BULK, RETRY_POLICY_DEFAULT["max_retries"], RETRY_POLICY_DEFAULT["interval_step"]), - (EMAIL_TYPE, NORMAL, RETRY_POLICY_DEFAULT["max_retries"], RETRY_POLICY_DEFAULT["interval_step"]), - (EMAIL_TYPE, PRIORITY, RETRY_POLICY_DEFAULT["max_retries"], RETRY_POLICY_DEFAULT["interval_step"]), - (SMS_TYPE, BULK, RETRY_POLICY_DEFAULT["max_retries"], RETRY_POLICY_DEFAULT["interval_step"]), - (SMS_TYPE, NORMAL, RETRY_POLICY_DEFAULT["max_retries"], RETRY_POLICY_DEFAULT["interval_step"]), - (SMS_TYPE, PRIORITY, RETRY_POLICY_HIGH["max_retries"], RETRY_POLICY_HIGH["interval_step"]), + (EMAIL_TYPE, BULK, RETRY_PERIODS[BULK]), + (EMAIL_TYPE, NORMAL, RETRY_PERIODS[NORMAL]), + (EMAIL_TYPE, PRIORITY, RETRY_PERIODS[NORMAL]), + (SMS_TYPE, BULK, RETRY_PERIODS[BULK]), + (SMS_TYPE, NORMAL, RETRY_PERIODS[NORMAL]), + (SMS_TYPE, PRIORITY, RETRY_PERIODS[PRIORITY]), ], ) - def test_delivery_task_parameters(self, notification_type, process_type, expected_retry, expected_retry_period): - params: Dict[str, Any] = build_delivery_task_params(notification_type, process_type) - assert params["retry"] is True - - retry_policy: Dict[str, Any] = params["retry_policy"] - assert retry_policy["max_retries"] == expected_retry - assert retry_policy["interval_start"] == expected_retry_period - assert retry_policy["interval_step"] == expected_retry_period - assert retry_policy["interval_max"] == expected_retry_period + def test_retry_task_parameters(self, notify_api, notification_type, process_type, expected_retry_period): + with notify_api.app_context(): + params: Dict[str, Any] = build_retry_task_params(notification_type, process_type) + + assert params["queue"] == QueueNames.RETRY + assert params["countdown"] == expected_retry_period def test_db_save_and_send_notification_throws_exception_when_missing_template(self, sample_api_key, mocker): mocker.patch("app.celery.provider_tasks.deliver_sms.apply_async")