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

Remove crates.downloads column #8233

Merged
merged 2 commits into from
Apr 12, 2024

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Mar 4, 2024

This PR builds on top of #8232. It switches the backend code to use the crate_downloads table for all downloads queries, and it subsequently drops the obsolete crates.downloads column.

This should be deployed separately from #8232!

@Turbo87 Turbo87 added C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear A-backend ⚙️ labels Mar 4, 2024
@Turbo87 Turbo87 requested a review from a team March 4, 2024 14:56
Copy link

codecov bot commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.60%. Comparing base (4065b2b) to head (7aca8d3).

❗ Current head 7aca8d3 differs from pull request most recent head 5613d70. Consider uploading reports for the commit 5613d70 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8233      +/-   ##
==========================================
- Coverage   87.88%   87.60%   -0.28%     
==========================================
  Files         270      272       +2     
  Lines       26092    26324     +232     
==========================================
+ Hits        22931    23062     +131     
- Misses       3161     3262     +101     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eth3lbert
Copy link
Contributor

I haven't started reviewing this PR yet. However, I noticed that index-related migration seems to be missing. If this is intentional, I think it would be helpful to include a comment in the migration script explaining the rationale behind this decision.

@bors
Copy link
Contributor

bors commented Mar 5, 2024

☔ The latest upstream changes (presumably 0c0ced7) made this pull request unmergeable. Please resolve the merge conflicts.

@Turbo87
Copy link
Member Author

Turbo87 commented Mar 5, 2024

I noticed that index-related migration seems to be missing. If this is intentional

good catch, that was indeed not intentional. but I just noticed that dropping the column automatically seems to remove the associated index. I guess you were asking about the down.sql that needs to add the index again?

@eth3lbert
Copy link
Contributor

eth3lbert commented Mar 5, 2024

Yes, and we should also consider adding an index on crate_downloads because there are sortings by downloads.

p.s. The index (downloads, crate_id) leads to an index only scan. An index (downloads) would result in an index scan.

@Turbo87 Turbo87 force-pushed the remove-crates-downloads-column branch 5 times, most recently from f10d866 to ddb6df0 Compare March 6, 2024 19:37
@Turbo87 Turbo87 force-pushed the remove-crates-downloads-column branch 4 times, most recently from d96cc6e to 2c3c19e Compare March 13, 2024 18:33
@Turbo87 Turbo87 force-pushed the remove-crates-downloads-column branch 3 times, most recently from 7aca8d3 to 83c7a52 Compare April 12, 2024 11:31
@Turbo87 Turbo87 marked this pull request as ready for review April 12, 2024 11:32
@Turbo87
Copy link
Member Author

Turbo87 commented Apr 12, 2024

@eth3lbert I've added the create index calls to the down.sql of the second migration

I think this is ready to go now 🤔

Turbo87 added 2 commits April 12, 2024 13:33
We no longer need to filter out updates to the `downloads` columns since that column is no longer being written to and will be removed in a follow-up commit.
@Turbo87 Turbo87 force-pushed the remove-crates-downloads-column branch from 83c7a52 to 5613d70 Compare April 12, 2024 11:33
Copy link
Contributor

@eth3lbert eth3lbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 LGTM 👍

@Turbo87 Turbo87 merged commit 0371322 into rust-lang:main Apr 12, 2024
6 checks passed
@Turbo87 Turbo87 deleted the remove-crates-downloads-column branch April 12, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants