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.

Sign up for GitHub

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

Jump to bottom

chore(github-growth): drop commitfilechange language column in django #56601

Merged
merged 4 commits into from
Sep 21, 2023
Merged

chore(github-growth): drop commitfilechange language column in django #56601

merged 4 commits into from
Sep 21, 2023

Conversation

cathteng
Copy link
Member

@cathteng cathteng commented Sep 20, 2023 •
edited
Loading

See #56491 for context

The original premise for writing to the CommitFileChange table to store the language associated with a change made in a commit was so that we could see the languages being used by organizations.

We could instead save the languages associated with commits in the Repository table, so we wouldn't need to write to CommitFileChange rows -- we also aren't interested in the number of changes that are being made in a particular language, just that folks are working in some language. The CommitFileChange table is also massive (1 billion plus rows) and querying is very slow.

After this I will add a migration to remove the column in Postgres.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 20, 2023
@cathteng cathteng marked this pull request as ready for review September 20, 2023 20:22
@cathteng cathteng requested a review from a team as a code owner September 20, 2023 20:22
@cathteng cathteng requested a review from a team September 20, 2023 20:22
@cathteng cathteng changed the title chore(github-growth): drop commitfilchange language column chore(github-growth): drop commitfilechange language column Sep 20, 2023
markstory
markstory reviewed Sep 20, 2023
View reviewed changes
src/sentry/migrations/0561_commitfilechange_drop_language_column.py Outdated
Comment on lines 26 to 29
migrations.RemoveField(
model_name="commitfilechange",
name="language",
),
Copy link
Member

Choose a reason for hiding this comment

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

This is an unsafe operation as it will cause failures in production when the column is removed whilst application code is using it. You'll need to remove the column from django state first, then remove the schema column. This develop guide has an example of what migrations should look like.

Copy link
Member Author

Choose a reason for hiding this comment

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

will it still be dangerous when following the process in the dev guide?

@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0561_commitfilechange_drop_language_column.py ()

--
-- Custom state/database change combination
--

@cathteng cathteng changed the title chore(github-growth): drop commitfilechange language column chore(github-growth): drop commitfilechange language column in django Sep 20, 2023
@codecov
Copy link

codecov bot commented Sep 20, 2023 •
edited
Loading

Codecov Report

Merging #56601 (2b14faa) into master (20677d5) will increase coverage by 26.92%.
The diff coverage is n/a.

❗ Current head 2b14faa differs from pull request most recent head 837b7ab. Consider uploading reports for the commit 837b7ab to get more accurate results

@@             Coverage Diff             @@
##           master   #56601       +/-   ##
===========================================
+ Coverage   51.70%   78.62%   +26.92%     
===========================================
  Files        5065     5083       +18     
  Lines      218566   219073      +507     
  Branches    37010    37093       +83     
===========================================
+ Hits       113017   172255    +59238     
+ Misses     103455    41249    -62206     
- Partials     2094     5569     +3475
Files Changed Coverage
src/sentry/models/commitfilechange.py ø

markstory
markstory approved these changes Sep 21, 2023
View reviewed changes
src/sentry/migrations/0561_commitfilechange_drop_language_column.py Outdated
# - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to
# have ops run this and not block the deploy. Note that while adding an index is a schema
# change, it's completely safe to run the operation after the code has deployed.
is_dangerous = True
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
is_dangerous = True
is_dangerous = False

State only changes don't need to be dangerous.

@cathteng cathteng force-pushed the cathy/github-growth/drop-commitfilechange-language-col branch from 2b14faa to 837b7ab Compare September 21, 2023 16:07
@github-actions
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0563_commitfilechange_drop_language_column.py ()

--
-- Custom state/database change combination
--

@cathteng cathteng enabled auto-merge (squash) September 21, 2023 16:18
@cathteng cathteng merged commit be6b9fc into master Sep 21, 2023
49 checks passed
@cathteng cathteng deleted the cathy/github-growth/drop-commitfilechange-language-col branch September 21, 2023 16:34
@cathteng cathteng mentioned this pull request Sep 21, 2023
Merged
michellewzhang pushed a commit that referenced this pull request Sep 21, 2023
@cathteng @michellewzhang
chore(github-growth): drop commitfilechange language column in django (…
aa8bf8e 
…#56601)
@github-actions github-actions bot locked and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@markstory markstory markstory approved these changes

Assignees
No one assigned
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cathteng @markstory