redis-rb 5.0 compatiblity #1828
Conversation
lib/resque/data_store.rb
Outdated
| @redis._client.connect | ||
| # Close the connection. Must be done after fork to avoid sharing the connection | ||
| # with the parent. | ||
| def disconnect! |
There was a problem hiding this comment.
Is there any way to retain the reconnect functionality? Changing the method name on a public method might necessitate more of a version bump than I was hoping for right now. Additionally, it's not clear if after calling disconnect! if we're going to reconnect at a later time.
There was a problem hiding this comment.
The Redis client always reconnected automatically.
If you want to force a reconnection, you can issue a command, e.g. PING.
There was a problem hiding this comment.
I added reconnect back as an alias, it's functionally equivalent unless somehow you 100% rely on the connection being create right there and not on use.
|
I merged your other PR into master so if you rebase and update this PR, it will clean up the diff. |
casperisfine
force-pushed
the
debug-reconnect
branch
from
698f61b
to
89d20c0
Compare
|
Rebased. |
casperisfine
force-pushed
the
debug-reconnect
branch
from
89d20c0
to
809606b
Compare
There was a problem hiding this comment.
Sorry to be a pain about changes on this PR... just trying to make sure that we're not impacting people who may have hooked into Resque and are expecting certain behaviors. I thought about it a lot over the course of the day and I think this is nearly there though.
lib/resque/worker.rb
Outdated
| # retry up to 3 times with increasing delay before giving up. | ||
| def reconnect |
There was a problem hiding this comment.
Same here with this one since it's still technically a public method. If we can make an alias or an alternative method that would be ideal.
Also in this case, does it make sense to attempt the PING after disconnecting in order to retain the reconnect/retry logic? This is the only place resque seems to do this and I'm wondering if removing it might result in reduced fault tolerance. If it only disconnects and then we rely on the redis client to handle reconnecting, you may find connection errors at interesting new spots in your backtrace that you weren't expecting.
There was a problem hiding this comment.
retain the reconnect/retry logic?
That reconnect/retry logic is totally redundant since Redis has it baked in, but ok. I honestly don't care, I don't use Resque.
There was a problem hiding this comment.
Yeah I hear ya... it was more out of concern for changing the behavior on public APIs since I know of other people using this that have various hooks and plugins that rely on this stuff. I appreciate you tweaking your PR though and I'm going to get a build going for early next week after Labor Day. Thanks!!
casperisfine
force-pushed
the
debug-reconnect
branch
from
809606b
to
c8e9d8a
Compare
There was two distinct issues. The first one was that `Resque::Worker` instances were passed as argument to `sadd / srem` relying on the implicit call to `to_s`. This has been removed in redis-rb 5.0, you now must explictly cast to a String or another type understood by the redis client. The other issue was with the reconnection after fork. When forking you must make sure to close the Redis connection, otherwise if you write in it, the responses of the parent and child will be mixed in. Resque was using a private API to do this reconnection. I updated it to use the main public API. There was also a bug in redis-rb 5.0.1 preventing from reconnecting after fork, I fixed it in 5.0.2.
casperisfine
force-pushed
the
debug-reconnect
branch
from
c8e9d8a
to
b5ff7e9
Compare
|
Done. I restored the behavior of |
* Update ErrorSubscriber's document * Lock redis version for CI See resque/resque#1828
Fix: #1828
Fix: #1825
Fix: #1820
Close: #1823
There was two distinct issues.
The first one was that
Resque::Workerinstances were passed as argument tosadd / sremrelying on the implicit call toto_s. This has been removed in redis-rb 5.0, you now must explictly cast to a String or another type understood by the redis client.The other issue was with the reconnection after fork. When forking you must make sure to close the Redis connection, otherwise if you write in it, the responses of the parent and child will be mixed in.
Resque was using a private API to do this reconnection. I updated it to use the main public API.
There was also a bug in redis-rb 5.0.1 preventing from reconnecting after fork, I fixed it in 5.0.2 (redis/redis-rb#1145).
NB: this is based on #1827 because the same lines are being edited so it avoid conflicts.