Do less work in update-downloads #1372
Merged
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
I was noticing that query to increment the downloads column on `crates` was getting run way more than I'd expect it to be. I also was seeing logs about a lock contention issue with that query. Specifically, those logs pointed me at one of two things wrong here. The first is that we were running this query even if we were incrementing the value by 0. Second, we have our cutoff set to 2 days ago. I think the reason for both of these is that we wanted to make sure the `processed` column got set to true. We can pretty easily avoid this by just setting it to true for all rows at the very end when all our preconditions are met. This doesn't necessarily fix the lock contention problem, but it does result in hitting far fewer rows, which should alleviate the problem.
jtgeibel
reviewed
Apr 27, 2018
| // against again. | ||
| diesel::update(version_downloads) | ||
| .set(processed.eq(true)) | ||
| .filter(date.lt(diesel::dsl::date(now))) |
There was a problem hiding this comment.
The comment says older than 24 hours ago, but I read this more as "yesterday or older in UTC". The behavior itself seems fine to me, but the comment confused me a bit at first. (As you mentioned the old behavior was 2 days ago, so the comment was confusing then as well.)
Other than that, I'm r+. This logic seems clearer and more efficient now and I think I finally understand how this portion of the logic works!
There was a problem hiding this comment.
Yeah, 24 hours is kinda wrong, but this matches what records will actually ever be touched again. After midnight UTC we never update old records.
|
bors: r+ |
bors-voyager bot
added a commit
that referenced
this pull request
Apr 27, 2018
1372: Do less work in update-downloads r=sgrif I was noticing that query to increment the downloads column on `crates` was getting run way more than I'd expect it to be. I also was seeing logs about a lock contention issue with that query. Specifically, those logs pointed me at one of two things wrong here. The first is that we were running this query even if we were incrementing the value by 0. Second, we have our cutoff set to 2 days ago. I think the reason for both of these is that we wanted to make sure the `processed` column got set to true. We can pretty easily avoid this by just setting it to true for all rows at the very end when all our preconditions are met. This doesn't necessarily fix the lock contention problem, but it does result in hitting far fewer rows, which should alleviate the problem.
Build succeeded |
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.
I was noticing that query to increment the downloads column on
crateswas getting run way more than I'd expect it to be. I also was seeing
logs about a lock contention issue with that query. Specifically, those
logs pointed me at one of two things wrong here.
The first is that we were running this query even if we were
incrementing the value by 0. Second, we have our cutoff set to 2 days
ago. I think the reason for both of these is that we wanted to make sure
the
processedcolumn got set to true. We can pretty easily avoid thisby just setting it to true for all rows at the very end when all our
preconditions are met.
This doesn't necessarily fix the lock contention problem, but it does
result in hitting far fewer rows, which should alleviate the problem.