Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

Do not fail task on notification failure #2435

Merged

Conversation

tevoinea
Copy link
Member

@tevoinea tevoinea commented Sep 22, 2022

Summary of the Pull Request

What is this about?

Closes #2422, today if we fail to process a notification queue message more than 5 times, the message ends up in a poison queue. But, we also fail the task associated with that notification and we don't want that behavior.

PR Checklist

  • Tests added/passed

Info on Pull Request

What does this include?

  • Renames fail_task to log_failed_notifications
  • log_failed_notifications no longer fails the task
  • log_failed_notifications includes the notification id in the error message for better debugging
  • teams webhook failure also includes the notification id
  • On the 5th failed attempt to process a queue_file_changes message, we call log_failed_notifications before moving the message to the poison queue

Related but not included in this PR: #2436

Validation Steps Performed

How does someone test & validate?

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2022

Codecov Report

Merging #2435 (bc0cae3) into main (b14bade) will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2435      +/-   ##
==========================================
+ Coverage   29.83%   29.88%   +0.05%     
==========================================
  Files         289      288       -1     
  Lines       35116    34993     -123     
==========================================
- Hits        10476    10458      -18     
+ Misses      24640    24535     -105     
Impacted Files Coverage Δ
src/ApiService/ApiService/Program.cs

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@tevoinea tevoinea marked this pull request as ready for review September 22, 2022 15:16
@tevoinea tevoinea added the refactor-diverge Used to indicate that python code - that has already been refactored - has been updated. label Sep 22, 2022
@tevoinea tevoinea enabled auto-merge (squash) September 22, 2022 20:57
@tevoinea tevoinea merged commit 4f9682d into microsoft:main Sep 22, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
refactor-diverge Used to indicate that python code - that has already been refactored - has been updated.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ADO notifications that fail because of a transient issue should be retried
4 participants