Merge Task Retry Handling & Performance#860
Open
EvanDietzMorris wants to merge 2 commits into
Open
Conversation
- separate retry functionality for errors from retries due to contention (failure to acquire a token or a db lock) - allow different max retry settings for each - make the backoff timing for normal retries constant/faster vs the exponential backoff for real errors - fix silent handling of merge lock retry exhaustion
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR aims to improve performance, resource utilization, and error reporting for the merge_and_post_process celery task queue implementation.
Issues Addressed
Currently, merge_and_post_process celery tasks are retried for three broad reasons: a failure to acquire an 'expensive token', failure to acquire a merge db lock, or when any Exception is raised during merge_and_post_process. All three of these share the same retry implementation, retry counter, and an exponential back off behavior that delays the next attempt by increasing amounts. The exponential back off delay is tuned to be quite slow, in an attempt to give downstream services time to recover in event of an error, but for normal 'expensive token' or db lock contention this isn't appropriate and results in unnecessary downtime when long back off delays occur while resources might already be available. Additionally, if deterministic errors occur, the current implementation still retries them up to the shared limit of retries even though they will fail again, wasting time and resources, with long delays in between.
The Solution
This PR separates the retry logic for Exceptions from retries due to contention, allowing us to tune them differently. By default it attempts retries due to contention much faster and for more attempts (every 2-5 seconds up to 100 times) and retries due to exceptions with the existing exponential back off (up to 30 seconds) but only for 5 attempts.
DB Lock Silent Failures
Here we also address a mismatch for how db lock retries were handled vs token contention. Previously the db lock had hard coded retry values inside the merge_and_post_process function (with a cap of 10 instead of 20), and when it was exhausted and gave up it silently caught the error with only a log message. Now the db lock contention retry shares the implementation of the token contention retry, and raises the failure when it occurs, allowing Celery and OTEL to recognize it.
New ENV Vars
This introduces two new env vars for tuning this retry logic, but does not attempt to alter the helm chart due to settings being applied in jenkins outside of this repo. That should be addressed in future work.
ARS_EXPENSIVE_TASK_MAX_RETRIES (the overall shared retry limit, default 100)
ARS_MERGE_ERROR_MAX_RETRIES (retry limit for real errors, default 5)
Misc.
This also changes a logging call in expensive_gate indicating a token acquisition is being attempted from warning to debug, to avoid clogging up logs and unnecessary log writes, because this is a normal operation that should happen all the time, and more frequently with these changes.
Future work
It is important to note that there are still significant improvements that could be made for error handling here. All exceptions are handled in the same way. Except for other logs outside of merge_and_post_process it is not apparent what kind of issue caused Exceptions or retries for them, which still causes deterministic errors to be retried unnecessarily, and hurts our ability to diagnose issues. Perhaps more importantly, according to Claude, errors that occur when hitting the node annotator or appraiser are swallowed and don't even cause Exceptions to be raised into the merge_and_post_process - so at least some of the kinds of errors that it is most appropriate to retry with exponential back off on probably aren't even being retried.