Ensure the update_downloads job doesn't run concurrently #2157
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
If multiple instances of this job are run concurrently then it is possible to overcount downloads, at least temporarily. The job first 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 overcount 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.
|
r? @sgrif (rust_highfive has picked a reviewer for you, use r? to override) |
|
r? @smarnach |
|
@bors r+ |
|
📌 Commit 0b03ae6 has been approved by |
bors
added a commit
that referenced
this pull request
Feb 6, 2020
Ensure the update_downloads job doesn't run concurrently If multiple instances of this job are run concurrently then it is possible to overcount downloads, at least temporarily. The job first 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 overcount 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.
|
☀️ Test successful - checks-travis |
jtgeibel
added a commit
to jtgeibel/crates.io
that referenced
this pull request
Feb 15, 2020
It appears that the additional wrapper transaction added around the update_downloads task causes delays and timeouts to download requests whenever the background job is run. Reverting so that master can be deployed. Revert "Auto merge of rust-lang#2157 - jtgeibel:add-lock-to-update-downloads-job, r=sgrif" This reverts commit c07223b, reversing changes made to c6d13eb.
jtgeibel
added a commit
to jtgeibel/crates.io
that referenced
this pull request
Mar 10, 2020
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
added a commit
to jtgeibel/crates.io
that referenced
this pull request
Mar 22, 2020
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.
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.
If multiple instances of this job are run concurrently then it is
possible to overcount downloads, at least temporarily. The job first
selects all matching
version_downloadsand later uses those values tocalculate how many downloads to add to
versionsandcrates. If asecond job is run, it would select some rows from
version_downloadsthat were already queued for processing by the first task.
If an overcount 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.