-
Notifications
You must be signed in to change notification settings - Fork 14
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
Remove the mixture of async endpoints with a sync db connection #901
Conversation
… causes issues as our database client not support async, so every database call introduces blocking calls to the event loop, eventually resulting in the workers timing out.
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
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.
Thanks @shangyian for catching this. @betodealmeida have you seen something like this before?
@shangyian seeing as the bottleneck has been the orm and db calls, should DJ move to async engines and sessions? @samredai and I were working on something and came out to approximately: def get_engine() -> AsyncEngine:
"""
Create the metadata engine.
"""
...
def getconn():
conn = connector.connect(instance_connection_name, "pg8000", user=db_user, password=db_pass, db=db_name)
return conn
engine = sqlalchemy.create_engine("postgresql+pg8000://", creator=getconn, future=True)
return engine as opposed to
|
@CircArgs yeah, I was investigating this in December and started working on a PR for it, but the necessary changes are actually huge: main...shangyian:dj:use-asyncpg The part where we just create an async engine is pretty easy, i.e., main...shangyian:dj:use-asyncpg#diff-5ebc477598017d1697e464ee79c28d2cdd36c55b2b904a358a8e5918ab4b2109R13. However, we also have to prevent implicit io which SQLAlchemy does whenever we dynamically pull in different objects on a given model: sqlalchemy/sqlalchemy#6165 (for example if we call Also fwiw I don't think bottleneck was database calls, but rather database calls that block the eventloop, which only happens when we issue sync calls from an async endpoint. We might eventually get to a point where database throughput is a problem, but we're not there yet (at least internally at Netflix, that is). |
@shangyian hmm sqlalchemy strikes again
😨 |
@CircArgs yeah it's probably also that we weren't using it in the most efficient way... but nonetheless, here we are :(
Yeaaaah unfun |
Summary
We should not mix async API endpoints with a synchronous db connection (
psycopg3
). Because our database client does not support async, every database call will introduce blocking calls to the event loop, eventually resulting in the uvicorn workers timing out.The issue is very similar to the one described here (in their case it was sync API call vs sync database query): tiangolo/uvicorn-gunicorn-fastapi-docker#145 (comment)
Test Plan
N/A
make check
passesmake test
shows 100% unit test coverageDeployment Plan
Ideally we deploy this right away. Otherwise over time the deployed DJ servers will experience more and more blocking calls, gradually causing workers to time out.