Allow download counts to fail to be updated #1675
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
This ensures that the download endpoint still works even if counting the download fails. The main case that we expect failure to occur is when the application is in read only mode. However, even if an unexpected failure occurs, we still want `cargo build` to succeed. Counting downloads is always considered optional -- it's much more important that people's builds succeed than having accurate download stats. Note that we still require a database connection from the pool. In theory, we could allow getting the version ID to fail as well, and just blindly redirect to S3 no matter what, and rely on a 404 happening there. However, this could result in successful builds in the event that the index is out of sync with our DB, since we don't routinely clean up the S3 bucket. Our current test behavior says that we 404 instead of redirecting, so I've left things as is. Once sfackler/r2d2#73 is released, my plan is to try to get a DB connection with a shorter timeout, and if we were able to get a DB connection, do our current behavior, but if that fails just blindly redirect to S3. We don't expect getting a connection to fail unless we're being DDOS'd, nor do we expect requests to this endpoint for crates that don't exist (since they should only come for things in the index). Fixes rust-lang#1387
|
I didn't test it locally, but this looks good to me. |
jtgeibel
requested changes
Mar 19, 2019
|
@bors r=jtgeibel |
|
📌 Commit c9a2b20 has been approved by |
bors
added a commit
that referenced
this pull request
Mar 19, 2019
Allow download counts to fail to be updated This ensures that the download endpoint still works even if counting the download fails. The main case that we expect failure to occur is when the application is in read only mode. However, even if an unexpected failure occurs, we still want `cargo build` to succeed. Counting downloads is always considered optional -- it's much more important that people's builds succeed than having accurate download stats. Note that we still require a database connection from the pool. In theory, we could allow getting the version ID to fail as well, and just blindly redirect to S3 no matter what, and rely on a 404 happening there. However, this could result in successful builds in the event that the index is out of sync with our DB, since we don't routinely clean up the S3 bucket. Our current test behavior says that we 404 instead of redirecting, so I've left things as is. Once sfackler/r2d2#73 is released, my plan is to try to get a DB connection with a shorter timeout, and if we were able to get a DB connection, do our current behavior, but if that fails just blindly redirect to S3. We don't expect getting a connection to fail unless we're being DDOS'd, nor do we expect requests to this endpoint for crates that don't exist (since they should only come for things in the index). Fixes #1387
|
☀️ Test successful - checks-travis |
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 ensures that the download endpoint still works even if counting the
download fails. The main case that we expect failure to occur is when
the application is in read only mode. However, even if an unexpected
failure occurs, we still want
cargo buildto succeed. Countingdownloads is always considered optional -- it's much more important that
people's builds succeed than having accurate download stats.
Note that we still require a database connection from the pool. In
theory, we could allow getting the version ID to fail as well, and just
blindly redirect to S3 no matter what, and rely on a 404 happening
there. However, this could result in successful builds in the event that
the index is out of sync with our DB, since we don't routinely clean up
the S3 bucket. Our current test behavior says that we 404 instead of
redirecting, so I've left things as is.
Once sfackler/r2d2#73 is released, my plan is to
try to get a DB connection with a shorter timeout, and if we were able
to get a DB connection, do our current behavior, but if that fails just
blindly redirect to S3. We don't expect getting a connection to fail
unless we're being DDOS'd, nor do we expect requests to this endpoint
for crates that don't exist (since they should only come for things in
the index).
Fixes #1387