-
Notifications
You must be signed in to change notification settings - Fork 329
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
Set community score to average of the questions. #1486
Set community score to average of the questions. #1486
Conversation
|
||
operations = [ | ||
migrations.RunSQL(SET_COMMUNITY_SCORE_TO_QUESTION_AVERAGE), | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If update_community_score()
where part of the Model or otherwise easily available outside the view itself, we could update the existing repository scores using the same code path you've already updated in this PR, ensuring no chance of a mismatch between two different implementations of the algorithm in different languages, one of which exists only in a migration and will only be run once. (Notoriously difficult to validate!)
This would also be a safer setup for future changes to the scoring algorithm, which has already changed a few times and surely will continue to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally try to avoid running Python in the migrations because it's really expensive to do so, but in this case it might not be a bad idea to make an exception to the rule. @cutwater thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts:
- Performing data migrations on can be bottleneck and affect service availability.
- Running python will affect performance drastically, because instead of single SQL we will end up with sending data back and forth within the single transaction.
- Re-using code in migration is dangerous, because code can eventually change that WILL break the migration.
- The best option in this particular case I think to create a background task to recalculate community scores and run it after migration.
|
||
operations = [ | ||
migrations.RunSQL(SET_COMMUNITY_SCORE_TO_QUESTION_AVERAGE), | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts:
- Performing data migrations on can be bottleneck and affect service availability.
- Running python will affect performance drastically, because instead of single SQL we will end up with sending data back and forth within the single transaction.
- Re-using code in migration is dangerous, because code can eventually change that WILL break the migration.
- The best option in this particular case I think to create a background task to recalculate community scores and run it after migration.
I decided to just do this in another PR. |
(cherry picked from commit 16ed4c1)
fixes: #1480