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

Insert/Upsert all notifications in one query during sync #482

Closed
wants to merge 12 commits into from
Closed

Insert/Upsert all notifications in one query during sync #482

wants to merge 12 commits into from

Conversation

andrew
Copy link
Member

@andrew andrew commented Oct 2, 2017

If we require Postgres 9.5 or greater we can make use of the on duplicate key update feature (also know as "upsert"), and put all the freshly synced notifications straight into the database in one query, rather than the many that we currently do, especially for new users.

I'm using the activerecord-import gem to do the heavy lifting, wiki page on on duplicate key update feature here: https://github.com/zdennis/activerecord-import/wiki/On-Duplicate-Key-Update

Couple of test failures atm which I think are related to updated_at timestamps not being set correctly

@andrew andrew added the enhancement Improvements and features label Oct 2, 2017
@chrisarcand
Copy link
Member

Nice, I was considering this as well when pondering about Heroku request timeouts. I didn't know that activerecord-import provides support for this, so that's nice.

@chrisarcand
Copy link
Member

chrisarcand commented Oct 4, 2017 •
edited

Couple of test failures atm which I think are related to updated_at timestamps not being set correctly

That's correct. I think it's because upserts are always considered 'changes', so the timestamp is updated for every insertion regardless of whether any data fields are actually changed. We're expecting the timestamps to stay the same in the tests.

Still investigating if

  1. We even care about that. After pondering further, pretty sure we care about updated_at. We sync that with the actual notification updated_at field, base some conditional update logic around it, sort with it...timestamps being set to the last time a notification was upserted (inserted or updated or attempted to be updated) would bork things up quite a bit.
  2. If we do, how we can avoid updating the timestamp if nothing changed.

Side note, reading the test output is confusing because we have our assertion order backwards (assert_equal expected, actual is what it should be...)

@andrew andrew self-assigned this Jan 10, 2018
@andrew andrew mentioned this pull request Jul 30, 2018
Closed
6 tasks
@andrew
Merge branch 'master' into upserts
e709f2b 
* master: (216 commits)
  Bump fugit from 1.1.4 to 1.1.5 (#708)
  Bump uglifier from 4.1.16 to 4.1.17 (#707)
  Handle null bytes gracefully (#705)
  Bump sidekiq-unique-jobs from 5.0.10 to 6.0.0 (#706)
  Add bugsnag js notifier (#704)
  Fixed spec
  Titlize label filter list name
  Bump et-orbi from 1.1.3 to 1.1.4 (#702)
  Bump mocha from 1.5.0 to 1.6.0 (#701)
  Bump rubocop from 0.58.1 to 0.58.2 (#700)
  Faster active user query in stats.rake
  Update README.md
  Update contact email address to new octobox.io one
  Bundled with bundler 1.16.3
  Bump uglifier from 4.1.15 to 4.1.16 (#698)
  Bump rack-test from 1.0.0 to 1.1.0 (#697)
  Only increase margin when filters are applied (#696)
  Bump fugit from 1.1.3 to 1.1.4 (#694)
  Bump rufus-scheduler from 3.5.0 to 3.5.1 (#695)
  Bump sass from 3.5.6 to 3.5.7 (#692)
  ...
@andrew
Merge branch 'master' into upserts
e5a6d81 
* master: (63 commits)
  Gracefully handle Octokit::ServerError exceptions caused by 504 response codes (#759)
  Bump attr_encrypted from 3.0.3 to 3.1.0 (#767)
  Bump octokit from 4.9.0 to 4.10.0 (#766)
  Bump sidekiq from 5.1.3 to 5.2.0 (#765)
  Bump uglifier from 4.1.17 to 4.1.18 (#764)
  Bump mocha from 1.6.0 to 1.7.0 (#763)
  Update screenshot on homepage and readme (#762)
  Fix colour of security_alert reason label
  Encrypt access tokens (#619)
  Add rack-canonical-host for redirecting to herokuapp subdomain to octobox.io (#760)
  Set production log level to :info
  Filter out personal_access_token and access_token from logs
  Bump sidekiq-unique-jobs from 6.0.5 to 6.0.6 (#758)
  Remove one more code climate readme badge
  Remove broken codeclimate badges from readme
  Set emoji height in labels to same height as text (#750)
  Improve the per-page dropdown menu (#744)
  Remove unread count from favicon when the unread count becomes zero (#746)
  Render emoji in labels on active filter menu (#751)
  Bump rails from 5.2.0 to 5.2.1 (#753)
  ...
@andrew
Merge branch 'master' into upserts
5478871 
* master:
  Bump git from 1.4.0 to 1.5.0 (#768)
@andrew
Merge branch 'master' into upserts
5200223 
* master:
  Test against postgres 9.6 on travis
@andrew andrew removed their assignment Aug 14, 2018
@andrew andrew added the blocked Waiting on something else before progressing label Aug 17, 2018
@andrew
Merge branch 'master' into upserts
c68a68a 
* master: (72 commits)
  Respect github_domain config when linking to notification settings (#832)
  Basic admin page (#833)
  Avoid using fetch_subject config method directly in views
  Add label scope to Subject
  Simpler relationship between repositories and subjects
  Add spacing between icon and text on login button
  Bump autoprefixer-rails from 9.1.2 to 9.1.3 (#830)
  Fix sync keyboard shortcut and animation
  Dark outline buttons should have white icons on focus as well
  Fix z-index of navbar dropdown
  Use git-merge icon for merged pull requests to match GitHub.com behavior (#827)
  Mark notifications as read when archiving (#826)
  Delete removed labels from subjects when syncing (#825)
  Document runtime-dyno-metadata setting in heroku setup
  Fixed misspellings of 'notification'
  Add missing documentation header
  Make the mute selected button available on archive view
  Pin the filter/action bar to the top of the notifications table (#821)
  Don't try to download subjects unless they are one of the four supported types (#819)
  Bump turbolinks from 5.1.1 to 5.2.0 (#822)
  ...
@andrew
Merge branch 'master' into upserts
a7909da 
* master: (81 commits)
  Set sidekiq log level to warn in production
  Avoid enqueing no-op subject syncing jobs
  Fixed doc link
  Fix active star colour in sidebar
  Sync button shouldn't refresh page if sync is happening in background (#884)
  Bump pg from 1.1.2 to 1.1.3 (#886)
  Fix display of current per_page option
  sync_notification jobs higher priority than sync_subject jobs
  Push subject syncing to sidekiq if background jobs enabled (#875)
  Add brakeman security scanner to development environment (#877)
  Remove excess per_page local variable
  Persist per page in cookie (#880)
  Refactor and improve test coverage on configurator class (#878)
  Remove unused codeclimate config file
  Bump simplecov from 0.13.0 to 0.16.1 (#881)
  Bump skylight from 2.0.2 to 3.0.0 (#879)
  Remove codeclimate reporting
  Bump fugit from 1.1.5 to 1.1.6 (#874)
  Bump et-orbi from 1.1.5 to 1.1.6 (#873)
  Avoid double sidebar separator when @States contains only a nil key (#872)
  ...
@andrew
Ensure self is returned
4a19cb1
@andrew
Copy link
Member Author

andrew commented Sep 7, 2018

This is going to need to be altered slightly to sync subjects and repositories after it's done the notification upsert as new notification records won't have an id before then, so can't be pushed into sidekiq until saved.

@andrew
Merge branch 'master' into upserts
50cb6a4 
* master: (35 commits)
  Show action buttons on hover of notification row (#823)
  Svg & card header fixes (#922)
  Addresses #919 Allow filtering by multiple repositories with search 
prefix (#921)
  * make all svg's $body-color (#920)
  Revert "Ignore the docs folder when generating docker images or heroku 
slugs" (#918)
  Dark mode (#769)
  Ignore the docs folder when generating docker images or heroku slugs 
(#916)
  Small tweaks to github app settings section (#915)
  Bump uglifier from 4.1.18 to 4.1.19 (#914)
  Bump octokit from 4.11.0 to 4.12.0 (#912)
  Don't show full list of repositories with app installed in settings 
(#911)
  Sync install permissions along with notifications (#909)
  Show installed github apps on user settings page (#910)
  Add vulnerability disclosure policy (#908)
  Sync GitHub App installations that a user has access to manage (#907)
  Bring back #891 (#905)
  Revert "Add sorting to notifications (#891)" (#904)
  Add sorting to notifications (#891)
  Add repository_full_name index on notifications table (#902)
  Bump bootsnap from 1.3.1 to 1.3.2
  ...
@andrew
Sync subjects and repositories after all notifications have been saved
77e8040
@andrew
Merge branch 'master' into upserts
225a1b2
@andrew
Copy link
Member Author

andrew commented Oct 12, 2018

I'm going to close this, I can't see it ever getting merged in this form, plus things are pretty quick at the moment so we're not in desperate need.

@andrew andrew closed this Oct 12, 2018
@andrew andrew deleted the upserts branch October 12, 2018 10:44
@andrew andrew removed the blocked Waiting on something else before progressing label Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisarcand chrisarcand Awaiting requested review from chrisarcand

Assignees
No one assigned
Labels
enhancement Improvements and features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@andrew @chrisarcand