Remove duplicate calls to db_conn in a single endpoint
#1757
Conversation
These endpoints are obtaining two separate connections for each request. If there is a backlog of requests, then these endpoints will wait in line multiple times for a database connection. Any request with an auth token or cookie will also obtain and drop a connection in the `CurrentUser` middleware. We could consider caching and reusing this database connection as well.
|
I agree that it feels "unfair" if a request needs to wait for a database connection multiple times, but I can't really tell what metric we are improving by reducing the number of connections obtained per request. The 99th percentile of latency, maybe? Do we have monitoring for that? Is the number of connections in the pool really a bottleneck that throttles processing in practice? If so, do we need to increase the pool size, rather than mitigating the impact of requests queuing up for database connections? Retrieving a connection from the pool is a very fast operation. The connection pool already is a cache for database connections. I would be reluctant to add the complexity of another level of cache per request unless we have data to prove that it is worth the trouble. That said, the changes in this particular PR look good to me. They may improve latency in some edge cases, and they don't really add complexity, so we can just as well merge them. I tested the endpoints manually for this branch merged into master, and they should also be covered by the tests. |
|
I agree, this is a micro-optimization. In production we did see an issue with a noisy neighbor that was saturating network activity, causing database requests to slow down. We ended up with 50 requests/threads contending for ~25 database connections (and likely with more requests queued in nginx). While I don't think this contention made the situation much worse, it does seem a bit odd to return a connection to the pool only to request a new one from the pool a few cycles later. On the other hand, I expect that there are some places where we should be dropping the connection and obtaining a new one later. Basically anywhere we block on an external service like GitHub or S3, allowing the connection to be reused rather than sit idle. @bors r=smarnach |
|
📌 Commit 4f605ef has been approved by |
Remove duplicate calls to `db_conn` in a single endpoint These endpoints are obtaining two separate connections for each request. If there is a backlog of requests, then these endpoints will wait in line multiple times for a database connection. Any request with an auth token or cookie will also obtain and drop a connection in the `CurrentUser` middleware. We could consider caching and reusing this database connection as well.
|
☀️ Test successful - checks-travis |
These endpoints are obtaining two separate connections for each
request. If there is a backlog of requests, then these endpoints will
wait in line multiple times for a database connection.
Any request with an auth token or cookie will also obtain and drop a
connection in the
CurrentUsermiddleware. We could consider cachingand reusing this database connection as well.