Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow download counts to fail to be updated #1675

Merged
merged 2 commits into from Mar 19, 2019

Conversation

Projects
None yet
4 participants
@sgrif
Copy link
Contributor

commented Mar 18, 2019

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

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
@bryanburgers

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

I didn't test it locally, but this looks good to me.

Show resolved Hide resolved src/tests/krate.rs
@sgrif

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

@bors r=jtgeibel

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

📌 Commit c9a2b20 has been approved by jtgeibel

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

⌛️ Testing commit c9a2b20 with merge b84b91a...

bors added a commit that referenced this pull request Mar 19, 2019

Auto merge of #1675 - sgrif:sg-resilient-crate-download, r=jtgeibel
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
@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

☀️ Test successful - checks-travis
Approved by: jtgeibel
Pushing b84b91a to master...

@bors bors merged commit c9a2b20 into rust-lang:master Mar 19, 2019

2 checks passed

homu Test successful
Details
percy/crates.io Visual review automatically approved, no visual changes found.
Details

@sgrif sgrif deleted the sgrif:sg-resilient-crate-download branch Mar 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.