-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
avoid/minimize more deadlocks #16343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@CAFxX is attempting to deploy a commit to the CLERKIEAI Team on Vercel. A member of the Team first needs to authorize it. |
e39fba6 to
5e9c787
Compare
ea2d6b3 to
2cdb95f
Compare
| data={"spend": {"increment": response_cost}}, | ||
| ) | ||
| break | ||
| except DB_CONNECTION_ERROR_TYPES as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why remove the retry logic? @CAFxX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the request does fail, what would happen ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@krrishdholakia the current retry logic seems to be broken... at least this is what our monitoring tells us (one single deadlock in the database immediately causes a spend exception, without any retry -- if the logic was working correctly there should be multiple deadlocks in a row before a single spend exception), and this seems to be confirmed by the definition of DB_CONNECTION_ERROR_TYPES (I actually do not understand at all what those httpx exceptions are, or why they are considered relevant here; I just know that the logic seems to not be working)
FWIW, I am not removing the retry logic; I just moved it to a single place (_handle_db_exception_retriable) - and hopefully fixed it as well to actually handle postgres deadlocks - since it's duplicated with small variations in so many different places
| if isinstance(e, DB_CONNECTION_ERROR_TYPES): # TODO: is this actually needed? | ||
| return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am keeping this just because it was the old logic. I have no idea, nor a way to practically test, if this is appropriate or not.
| # Unfortunately there does not seem to be a easy way to properly parse that or otherwise detect the specific | ||
| # issue, so just match using a regular expression. This is definitely not ideal, but not much we can do about | ||
| # it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least, I could not find a better way. Suggestions are most welcome.
Title
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unitType
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes