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
fix: opening & closing many itx should not increase time #3028
Conversation
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.
Really nice job. I've given to very minor naming nits. But I think where we check this list needs to be moved.
query-engine/core/src/interactive_transactions/actor_manager.rs
Outdated
Show resolved
Hide resolved
query-engine/core/src/interactive_transactions/actor_manager.rs
Outdated
Show resolved
Hide resolved
mut rx: Receiver<TxId>, | ||
) -> JoinHandle<()> { | ||
tokio::task::spawn(async move { | ||
loop { | ||
if let Some(id) = rx.recv().await { | ||
trace!("removing {} from client list", id); | ||
clients.write().await.remove(&id); | ||
|
||
let mut guard = clients.write().await; |
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.
Another minor nit. This is a guard and the actual Hashmap. So a better name is client_map
or map
.
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.
Renamed it to clients_guard
, since we're dropping the guard, not the map, right?
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.
Yeah that is good. Its a guard that wraps the map.
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.
Really nice fix. Thank you
Make the error messages for closed transactions more detailed, add extra context about the transaction timeout. Before: - `Transaction API error: Transaction already closed: A query/commit/rollback cannot be executed on a closed transaction..` (also note the double "." at the end) Now: - `Transaction API error: Transaction already closed: A query/commit/rollback cannot be executed on a committed transaction.` - `Transaction API error: Transaction already closed: A query/commit/rollback cannot be executed on a transaction that was rolled back.` - `Transaction API error: Transaction already closed: A query/commit/rollback cannot be executed on an expired transaction. The timeout for this transaction was X ms, however Y ms passed since the start of the transaction. Consider increasing the interactive transaction timeout or doing less work in the transaction.` Additionally, the "Transaction not found error" is now also more verbose, ref: https://www.notion.so/disconnect-with-iTX-f3cfee3ff4924e40aa90aadb2454e9fa?d=3bd7c7103b02461bbfe414a978a994c1#547ab127682b41898c87bdd5c841c0bf Also contains minor cleanup things related to iTX: * Remove the unused `CachedTx::Aborted` variant * Remove references to an obsolete env var that doesn't exist since #3028 from comments and `.envrc` Client PR: prisma/prisma#16382 Closes: prisma/prisma#13713 Ref: prisma/prisma#16050 Ref: #3028
Overview
fixes prisma/prisma#12516
The clippy issues don't seem related to my changes