Ensure the update_downloads job doesn't run concurrently #2266
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jtgeibel
commented
Mar 10, 2020
| match diesel::select(pg_advisory_unlock(self.key)).get_result(self.conn) { | ||
| Ok(true) => (), | ||
| Ok(false) => println!( | ||
| "Error: job advisory lock for key {} was not locked", |
There was a problem hiding this comment.
Maybe this should be a panic, to forbid the callback from releasing the lock on its own.
jtgeibel
commented
Mar 10, 2020
| "Error: job advisory lock for key {} was not locked", | ||
| self.key | ||
| ), | ||
| Err(err) => println!("Error unlocking advisory lock (key: {}): {}", self.key, err), |
There was a problem hiding this comment.
Similar here. We could do something (or just panic) to ensure the connection is closed and the lock unlocked.
|
☔ The latest upstream changes (presumably #2269) made this pull request unmergeable. Please resolve the merge conflicts. |
This is an improved implementation of rust-lang#2157. The previous design relied on a transaction based lock to manage the lifetime of the lock. The wrapper transaction caused the `update_downloads` job to interfere with incoming download requests, and the changes had to be reverted. This implementation uses a session lock which is automatically released even if the callback panics. If multiple instances of the `update_downloads` job are run concurrently then it is possible to over count downloads, at least temporarily. The first job selects all matching `version_downloads` and later uses those values to calculate how many downloads to add to `versions` and `crates`. If a second job is run, it would select some rows from `version_downloads` that were already queued for processing by the first task. If an over count were to occur, the next time the job is run it should calculate a negative adjustment and correct the situation. There's no point in doing extra work and if we eventually need concurrency we should built that out intentionally. Therefore, this commit wraps the entire job in a transaction and obtains an transaction level advisory lock from the database. If the lock has already been taken the job will fail and will be retried by swirl. If the duration of this job begins to approach the scheduling interval, then we will want to increase that interval to avoid triggering alerts.
jtgeibel
force-pushed
the
add-session-lock-for-update-downloads-job
branch
from
0d62fe1 to
4be63c4
Compare
|
☔ The latest upstream changes (presumably #2328) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing as superseded by #2539. We've now seen that fix work correctly several times in production. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an improved implementation of #2157. The previous design
relied on a transaction based lock to manage the lifetime of the lock.
The wrapper transaction caused the
update_downloadsjob to interferewith incoming download requests, and the changes had to be reverted.
This implementation uses a session lock which is automatically released
even if the callback panics.
r? @sgrif