update logging in award program certificate task - #16822
Conversation
tasawernawaz
changed the title
6c8d6e7 to
87b9f61
Compare
tasawernawaz
changed the title
| # keep trying to award other certs, but retry the whole task to fix any missing entries | ||
| LOGGER.exception('Failed to award certificate for program %s to user %s', program_uuid, username) | ||
| retry = True | ||
| error_message = 'Failed to award certificate for program %s to user %s', program_uuid, username |
There was a problem hiding this comment.
move this inside if block
|
|
||
| with mock.patch(TASKS_MODULE + '.LOGGER.info') as mock_info, \ | ||
| mock.patch(TASKS_MODULE + '.LOGGER.exception') as mock_exception: | ||
| mock.patch(TASKS_MODULE + '.LOGGER.warning') as mock_exception: |
There was a problem hiding this comment.
also add test that after maximum retries exception is logged
87b9f61 to
f657e63
Compare
|
@ahsan-ul-haq Addressed your comments. Please review it again. |
ahsan-ul-haq
left a comment
ahsan-ul-haq
left a comment
There was a problem hiding this comment.
One small comment rest looks fine to me
There was a problem hiding this comment.
Lets call this warning_message and move it inside if block as it is the only place it is used
There was a problem hiding this comment.
I will change it, also this is used in else block as well.
f657e63 to
a6f4f17
Compare
waheedahmed
left a comment
waheedahmed
left a comment
There was a problem hiding this comment.
Just one comment, otherwise LGTM
| # keep trying to award other certs, but retry the whole task to fix any missing entries | ||
| LOGGER.exception('Failed to award certificate for program %s to user %s', program_uuid, username) | ||
| retry = True | ||
| warning_message = 'Failed to award certificate for program {uuid} to user {username}.'.format( |
There was a problem hiding this comment.
I think it would be best if you add retrying at the end as well Failed to award certificate for program {uuid} to user {username}, retrying. It will add more clarity about this warning.
There was a problem hiding this comment.
we have a info message below at this line for retrying.
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Tuesday, December 12, 2017. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
| warning_message = 'Failed to award certificate for program {uuid} to user {username}.'.format( | ||
| uuid=program_uuid, username=username) | ||
|
|
||
| if countdown < MAX_RETRIES: |
There was a problem hiding this comment.
@tasawernawaz That's not how this works. The retry method handles this logic for you.
countdown is a time delay and MAX_RETRIES is a number of attempts. These cannot be compared.
http://docs.celeryproject.org/en/latest/reference/celery.app.task.html#celery.app.task.Task.retry
There was a problem hiding this comment.
@bderusha if retry logic already handling this. I think I only need to log message as warning, once retries are exceeded, error will be logged from retry logic ?
5 participants
LEARNER-3165
@edx/learner-spartans Please review.