From cfe82f8f4a13df196e0b26637b3e5f17e06a025b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 14 Nov 2019 10:52:24 +0000 Subject: [PATCH] make 500 error provider switches also check for recent changes moving the logic and the test from switch provider on slow delivery to dao reduce sms provider priority --- app/celery/scheduled_tasks.py | 8 ------- app/dao/provider_details_dao.py | 27 ++++++++++++++++++---- tests/app/celery/test_scheduled_tasks.py | 18 +-------------- tests/app/dao/test_provider_details_dao.py | 15 ++++++++++++ 4 files changed, 38 insertions(+), 30 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index b67d3e61b..297713ebd 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -35,7 +35,6 @@ from app.dao.notifications_dao import ( ) from app.dao.provider_details_dao import dao_reduce_sms_provider_priority from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago -from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.models import ( Job, JOB_STATUS_IN_PROGRESS, @@ -111,13 +110,6 @@ def switch_current_sms_provider_on_slow_delivery(): in the last ten minutes. If both providers are slow, don't do anything. If we changed the providers in the last ten minutes, then don't update them again either. """ - providers = get_provider_details_by_notification_type('sms') - # if something updated recently, don't update again. If the updated_at is null, set it to min time - # just to prevent errors - if any((provider.updated_at or datetime.min) > datetime.utcnow() - timedelta(minutes=10) for provider in providers): - current_app.logger.info("Slow delivery notifications provider switched less than 10 minutes ago.") - return - slow_delivery_notifications = is_delivery_slow_for_providers( threshold=0.3, created_at=datetime.utcnow() - timedelta(minutes=10), diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index bb130ffe1..878e8c23d 100644 --- a/app/dao/provider_details_dao.py +++ b/app/dao/provider_details_dao.py @@ -1,4 +1,4 @@ -from datetime import datetime +from datetime import datetime, timedelta from notifications_utils.timezones import convert_utc_to_bst from sqlalchemy import asc, desc, func @@ -36,30 +36,47 @@ def dao_get_provider_versions(provider_id): @transactional def dao_reduce_sms_provider_priority(identifier): - # TODO: do we want to hold off on reducing priority if we've already adjusted priorities recently? - # do we want to do anything differently between slow delivery vs 500s? - # get current priority of both providers q = ProviderDetails.query.filter( ProviderDetails.notification_type == 'sms', ProviderDetails.active - ).with_for_update() + ).with_for_update().all() providers = {provider.identifier: provider for provider in q} other_identifier = get_alternative_sms_provider(identifier) + # if something updated recently, don't update again. If the updated_at is null, treat it as min time + if any((provider.updated_at or datetime.min) > datetime.utcnow() - timedelta(minutes=10) for provider in q): + current_app.logger.info("Not adjusting providers, providers updated less than 10 minutes ago.") + return + reduced_provider = providers[identifier] increased_provider = providers[other_identifier] + pre_reduction_priority = reduced_provider.priority + pre_increase_priority = increased_provider.priority # always keep values between 0 and 100 reduced_provider.priority = max(0, reduced_provider.priority - 10) increased_provider.priority = min(100, increased_provider.priority + 10) + current_app.logger.info('Adjusting provider priority - {} going from {} to {}'.format( + reduced_provider.identifier, + reduced_provider.priority, + pre_reduction_priority + )) + current_app.logger.info('Adjusting provider priority - {} going from {} to {}'.format( + increased_provider.identifier, + increased_provider.priority, + pre_increase_priority + )) + # Automatic update so set as notify user notify_user = get_user_by_id(current_app.config['NOTIFY_USER_ID']) reduced_provider.created_by_id = notify_user.id increased_provider.created_by_id = notify_user.id + # update without commit so that both rows can be changed without ending the transaction + # and releasing the for_update lock _update_provider_details_without_commit(reduced_provider) _update_provider_details_without_commit(increased_provider) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 5fa11eebb..fe160a7ea 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -21,7 +21,7 @@ from app.celery.scheduled_tasks import ( from app.config import QueueNames, TaskNames from app.dao.jobs_dao import dao_get_job_by_id from app.dao.notifications_dao import dao_get_scheduled_notifications -from app.dao.provider_details_dao import get_provider_details_by_notification_type, get_provider_details_by_identifier +from app.dao.provider_details_dao import get_provider_details_by_identifier from app.models import ( JOB_STATUS_IN_PROGRESS, JOB_STATUS_ERROR, @@ -122,21 +122,6 @@ def test_switch_current_sms_provider_on_slow_delivery_switches_when_one_provider mock_reduce.assert_called_once_with('firetext') -@freeze_time('2017-05-01 14:00:00') -def test_switch_current_sms_provider_on_slow_delivery_does_nothing_if_recent_changes( - mocker, - restore_provider_details, -): - mock_is_slow = mocker.patch('app.celery.scheduled_tasks.is_delivery_slow_for_providers') - mock_reduce = mocker.patch('app.celery.scheduled_tasks.dao_reduce_sms_provider_priority') - get_provider_details_by_identifier('mmg').updated_at = datetime(2017, 5, 1, 13, 51) - - switch_current_sms_provider_on_slow_delivery() - - assert mock_is_slow.called is False - assert mock_reduce.called is False - - @freeze_time('2017-05-01 14:00:00') @pytest.mark.parametrize('is_slow_dict', [ {'mmg': False, 'firetext': False}, @@ -151,7 +136,6 @@ def test_switch_current_sms_provider_on_slow_delivery_does_nothing_if_no_need( mock_reduce = mocker.patch('app.celery.scheduled_tasks.dao_reduce_sms_provider_priority') get_provider_details_by_identifier('mmg').updated_at = datetime(2017, 5, 1, 13, 51) - switch_current_sms_provider_on_slow_delivery() assert mock_reduce.called is False diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index a985510e2..8ffbc3730 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -185,6 +185,21 @@ def test_reduce_sms_provider_priority_adds_rows_to_history_table( assert updated_provider_history_rows[0].priority == 90 +@freeze_time('2017-05-01 14:00:00') +def test_reduce_sms_provider_priority_does_nothing_if_providers_have_recently_changed( + mocker, + restore_provider_details, +): + mock_is_slow = mocker.patch('app.celery.scheduled_tasks.is_delivery_slow_for_providers') + mock_reduce = mocker.patch('app.celery.scheduled_tasks.dao_reduce_sms_provider_priority') + get_provider_details_by_identifier('mmg').updated_at = datetime(2017, 5, 1, 13, 51) + + dao_reduce_sms_provider_priority('firetext') + + assert mock_is_slow.called is False + assert mock_reduce.called is False + + @freeze_time('2018-06-28 12:00') def test_dao_get_provider_stats(notify_db_session): service_1 = create_service(service_name='1')