persist current auction in background task #3861
Conversation
MartinquaXD
changed the title
| id: domain::auction::Id, | ||
| auction: &domain::RawAuctionData, |
There was a problem hiding this comment.
I think the variables could have better names, it's kind of hard to tell which is which by just looking at it. Is the ID the new one, or is it contained in AuctionData?
| let mut ex = self.pool.acquire().await?; | ||
| let id = database::auction::replace_auction(&mut ex, &data).await?; | ||
| Ok(id) | ||
| database::auction::insert_auction_with_id(&mut ex, id, &data).await?; |
There was a problem hiding this comment.
To supplement my point, I'd have to dive up to here to understand that idis the new auction ID
| Ok(key) => tracing::info!(?key, "uploaded auction to s3"), | ||
| Err(err) => tracing::warn!(?err, "failed to upload auction to s3"), |
There was a problem hiding this comment.
Should we add the auction id to the logs too? I'm not sure the error has it
There was a problem hiding this comment.
The upload key already contains the auction id so I'd leave it as is.
squadgazzz
left a comment
squadgazzz
left a comment
There was a problem hiding this comment.
That change looks a bit risky and awkward to me. How about introducing a simple table with an auction_id counter as the safest approach?
|
I think there is a potential issue with ordering - it could happen, however unlikely, that you spawn a taks T1, then T2, but T2 finishes before T1 and writes T1 writes over a newer auction. 🤔 |
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
In what way would having a new table be less risky than using the already existing counter? Or do you see the risk in moving the upload to a background task? Other than ☝️ I (hopefully) improved the variable naming to make it more clear and addressed the comment about uploads happening out of order. |
squadgazzz
left a comment
squadgazzz
left a comment
There was a problem hiding this comment.
It was a long time ago, and I can't remember the case I was thinking about. Right now I don't really see any issue, since this operation is executed sequentially here
services/crates/autopilot/src/run_loop.rs
Lines 170 to 172 in 914055c
Apologies for the back-and-forth.
Description
Currently the autopilot assembles the contents of the auction and then persists it in the DB by replacing the current auction with the new one which also returns the ID for the new auction.
This is unfortunately pretty slow (~440ms on prod mainnet) and therefor significantly increases the delay between seeing a new block and sending the fully assembled auction to solvers.
Changes
What we can do instead is to introduce a new DB query that just increments and returns the auction id counter for the next auction. Afterwards we spawn a background task that writes the fully assembled auction to the DB and uploads it to S3.
The trade-off is that it's now possible that we fully run an auction that we don't have persisted anywhere. But in practice this does not happen (no instance in the last 30d) and this information is for debugging purposes anyway.
How to test
e2e tests