detect Rails DB connection and use it #232
Conversation
|
A quick review of this code would be welcomed :) |
|
@jipiboily I'm traveling right now. I can take a look when I get back in a few days. |
|
It looks like the build is failing on some Ruby version...will need to fix that first. Thanks! :) |
jipiboily
changed the title
|
@senny @ryandotsmith @ukd1 @smathieu could one of you review this please? It works, just tested with migrations + running a job that uses a model. I would love to get that merged as soon as possible. ❤️ |
| end | ||
|
|
||
| def after_teardown | ||
| ensure |
There was a problem hiding this comment.
Why do you need the unsure?
There was a problem hiding this comment.
Good question! I don't remember...I'll test and if it's required, I'll document the reason.
|
How do you turn this off if that's not what you want? |
|
Good point about disabling it, I'll add a ENV var to disable it and add to README. |
|
@smathieu updated. Removed the useless |
|
Going to merge within a few minutes. |
detect Rails DB connection and use it
|
@jipiboily I know I'm late to the party but does hits patch address the issues raised in #141 and #96 ? |
|
Hum, not on purpose, I'll have to read about those threads fully. Regarding #141, we are using AC's connection and never had any problem. We don't use this PR though... I'll read through later today or tomorrow and report back. |
|
thanks man. The linked issues were always the case why using the AR connection was never fully supported. |
|
@senny I'm totally overloaded in both my personal/family and work lifes...so not sure when I'll have even 5 minutes to dedicate to open source :/ |
|
@senny I can't reproduce TBH and I am not sure what to do here. This PR includes a way of disabling the auto use of AR's connection, so it should provide a around at least. Until we have more information or a repro case, I think it's hard to do anything else... FWIW, we processed over 80 millions of jobs so far and I've never seen this error. #141 was using older versions of Rails and QC, so maybe it is not a problem anymore? |
|
@jipiboily I'm fine with waiting for problem reports. If it's still an issue I'm sure we'll get reports if it happens again. |
This is to make it as easy to use queue_classic with Rails, especially with Rails 4.2 + ActiveJob getting out soon-ish.
This is a work in progress. Any help on getting a test for that working would be more than welcomed as I am minitest noob and don't have much more time today for that.
Thoughts on how to test that with minitest?
Related to: #228
TODO:
add CHANGELOG entry