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

Replaces thread storage with request_store #218

Merged
merged 1 commit into from Aug 4, 2017

Conversation

@leonelgalan
Copy link
Contributor

@leonelgalan leonelgalan commented Aug 3, 2017

Fixes issue #197. Under load it was possible for a thread to handle a second request without clearing the variables in the thread. This caused log lines to have the incorrect values for : lograge_location.

@abrisse suggested we used request_store, after a brief examination of this gem it seems that it’s well maintained and it solves our problem.

Fixes issue #197. Under load it was possible for a thread to handle
a second request without clearing the variables in the thread. This
caused log lines to have the incorrect values for `: lograge_location`.

@abrisse suggested we used `request_store`, after a brief examination
of this gem it seems that it’s well maintained and it solves our
problem.
@abrisse
Copy link

@abrisse abrisse commented Aug 4, 2017

LGTM

@pxlpnk pxlpnk requested review from benlovell and pxlpnk Aug 4, 2017
@pxlpnk
Copy link
Collaborator

@pxlpnk pxlpnk commented Aug 4, 2017

Looks good, I am just wondering about the sidekiq issue mentioned here:
https://github.com/steveklabnik/request_store/pull/54/files

I have no numbers though how many people use lograge with sidkiq.
@abrisse @leonelgalan do you run a sidekiq setup somewhere that utilises lograge?

@benlovell
Copy link
Collaborator

@benlovell benlovell commented Aug 4, 2017

@pxlpnk I'm not sure that's a problem here. As far as I can tell Active Job and Sidekiq workers don't use our subscriber.

@pxlpnk
pxlpnk approved these changes Aug 4, 2017
@pxlpnk
Copy link
Collaborator

@pxlpnk pxlpnk commented Aug 4, 2017

Then this approach is definitely more sane and safe 👍

@benlovell benlovell merged commit 485dfc3 into roidrage:master Aug 4, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benlovell
Copy link
Collaborator

@benlovell benlovell commented Aug 4, 2017

Thanks! I'll cut a release today.

@reedloden

This comment has been minimized.

Copy link

@reedloden reedloden commented on lograge.gemspec in b6ed2a8 Aug 9, 2017

Why 1.0 here? Current is 1.3.2, which this doesn't match (due to the twiddle-wakka). Can this be changed to '>= 1.0" or something?

@pxlpnk
Copy link
Collaborator

@pxlpnk pxlpnk commented Aug 9, 2017

I can't see a specific reason, why we would pin it to 1.0.

Lets update it to the latest: #219

@leonelgalan
Copy link
Contributor Author

@leonelgalan leonelgalan commented Aug 17, 2017

Sorry for the late response, I left it at '~> 1.0', because (1) it resolves to the latest, and (2) we don't use anything introduced after 1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants