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

Fix create rollup tables #35403

Merged
merged 3 commits into from
Jun 19, 2020
Merged

Fix create rollup tables #35403

merged 3 commits into from
Jun 19, 2020

Conversation

davidsbailey
Copy link
Member

@davidsbailey davidsbailey commented Jun 19, 2020

This commit Faster only_one exit sure made this cronjob complete quickly! 😝

Dave-MBP:~/src/cdo (staging)$ ./bin/cron/create_rollup_tables 
Traceback (most recent call last):
	2: from ./bin/cron/create_rollup_tables:6:in `<main>'
	1: from /Users/dsb/.rbenv/versions/2.5.0/lib/ruby/2.5.0/rubygems/core_ext/kernel_require.rb:59:in `require'
/Users/dsb/.rbenv/versions/2.5.0/lib/ruby/2.5.0/rubygems/core_ext/kernel_require.rb:59:in `require': 
cannot load such file -- cdo/only_one (LoadError)

This problem was introduced in #32091 on Dec 2, 2019. To confirm that I wasn't just having this problem locally, I looked at the contained_levels table in the staging DB and confirmed that a level group created before December does appear there, and a level group created after December does not.

Why didn't we catch this? It's fair to say that every single person on our team has missed it, because the cronjob has been failing every time it's run since December, and an error has been logged to HoneyBadger every time: https://app.honeybadger.io/projects/45435/faults?sort=last_seen_desc&q=create_rollup_tables

Future work

  • make sure these kinds of errors are getting noticed in HoneyBadger by whoever is oncall.

Testing story

Manually ran this cronjob locally. Before this change it generated no rows:

mysql> select count(*) from contained_levels;
+----------+
| count(*) |
+----------+
|        0 |
+----------+

after this change, it generates:

mysql> select count(*) from contained_levels;
+----------+
| count(*) |
+----------+
|     3529 |
+----------+

on the staging machine, this table has only 2620 entries. The higher number generated locally is probably due to all the level groups created by cloning the 2019 curriculum to create the 2020 curriculum, which generally happens between January and March.

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

frustrated-gary

Wow, I can't believe how bad I broke this. Thank you for noticing the error and fixing this script!

@@ -3,11 +3,10 @@
# This script populates (after truncating) the contained_levels,
# contained_level_answers, and the level_sources_multi_type tables.

require_relative '../../dashboard/config/environment'
require 'cdo/only_one'
exit unless only_one_running?(__FILE__)
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're in here, I think we should follow Will's lead and convert the only_one require into a require_relative, instead of doing the slow environment load before this check.

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidsbailey davidsbailey merged commit cddd483 into staging Jun 19, 2020
@davidsbailey davidsbailey deleted the fix-create-rollup-tables branch June 19, 2020 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants