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

Add a new session type for job queues #84

Merged
merged 7 commits into from
Aug 27, 2019
Merged

Conversation

aburgel
Copy link
Collaborator

@aburgel aburgel commented May 27, 2019

/domain @Betterment/test_track_core
/no-platform

This introduces a JobSession (along with a DelayedJob plugin that makes use of it) that manages the lifecycle and keeps a cache of visitors. This improves performance by eliminating duplicate identity lookups within a job.

This addresses some of #55, but it keeps the OfflineSession for unmanaged contexts, like the command line and for looking up visitors by visitor ID.

I renamed the existing Session model to WebSession for clarity, this is much of the diff. There's a tiny bit of overlap between JobSession and SessionVisitorRepository, but it was only a few lines, so I opted for duplication rather than refactoring.

@nanda-prbot
Copy link

Needs somebody from @Betterment/test_track_core to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

@@ -1,7 +1,4 @@
require 'delayed_job'
require 'delayed_job_active_record'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure why we had these here. We require delayed_job in the engine, so this seems unnecessary. I'll see if any apps blow up with this change before merging.

@jmileham
Copy link
Member

Cool, if this doesn't cause any test leakage or other issues with the require statements you pulled << domain lgtm! Thanks so much!

@nanda-prbot
Copy link

Approved! 🎆 ⛳ 🎇

@aburgel
Copy link
Collaborator Author

aburgel commented Aug 27, 2019

if this doesn't cause any test leakage or other issues with the require statements you pulled

ran a couple of app test suites against this, and all looks good.

@aburgel aburgel merged commit ed7672c into master Aug 27, 2019
@aburgel aburgel deleted the aburgel/background-session branch August 27, 2019 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants