Non-blocking Redis calls: add an async worker #92
Conversation
davidor
force-pushed
the
async-redis-worker
branch
from
a1f100e
to
49b0e64
Compare
davidor
force-pushed
the
async-redis-worker
branch
from
49b0e64
to
c04dc54
Compare
|
Rebased on top of the |
davidor
force-pushed
the
async-redis-worker
branch
from
3f176a1
to
66c0f7c
Compare
| :max_pending_jobs, | ||
| # Seconds to wait before fetching more jobs when the number of jobs | ||
| # in memory has reached max_pending_jobs. | ||
| :seconds_before_fetching_more |
There was a problem hiding this comment.
Not sure if it's worth it to add an extra dependency for that.
There was a problem hiding this comment.
Sorry I meant the rename it to throttling
There was a problem hiding this comment.
👍
I misunderstood you 😅
There was a problem hiding this comment.
I thought about this, but in the end I decided to keep the original name because I think it's more explicit. Throttling is more generic, depending on the algorithm used it can refer to number of requests per unit of time, that plus a burst rate, etc.
lib/3scale/backend/job_fetcher.rb
Outdated
| private_constant :DEFAULT_WAIT_BEFORE_FETCHING_MORE_JOBS | ||
|
|
||
| class RedisConnectionError < RuntimeError | ||
| def initialize(msg) |
There was a problem hiding this comment.
If what it is doing is only super, do you need it?
lib/3scale/backend/job_fetcher.rb
Outdated
| else | ||
| raise e | ||
| end | ||
| rescue => e |
There was a problem hiding this comment.
rescue and raise the exception is not needed here.
| encoded_job[0].sub!('queue:', '') | ||
| Resque::Job.new(encoded_job[0], | ||
| Yajl::Parser.parse(encoded_job[1], check_utf8: false)) | ||
| rescue Exception => e |
There was a problem hiding this comment.
Do you need to rescue all Exception here? Even signals?
If so please add a note.
There was a problem hiding this comment.
Signals are trapped in a different file.
If something happens while decoding the job we do not want the program to exit, and we want to log what happened, so I think that the current solution is OK. There's a brief explanation just below this line.
|
|
||
| # redis-rb accepts a Hash as last arg that can contain :timeout. | ||
| if call_args.last.is_a? Hash | ||
| timeout = call_args.last[:timeout] |
There was a problem hiding this comment.
| timeout = call_args.last[:timeout] | |
| timeout = call_args.pop[:timeout] |
| # redis-rb accepts a Hash as last arg that can contain :timeout. | ||
| if call_args.last.is_a? Hash | ||
| timeout = call_args.last[:timeout] | ||
| call_args.pop |
There was a problem hiding this comment.
| call_args.pop |
I forgot to add this method in previous PRs. This method needs to be treated a bit differently because redis-rb accepts a hash that contains a timeout. We need to get that timeout and simply send it as the last param of the list.
And transform Worker into a module that contains common methods for other workers.
While at it, remove TODO about stubbing the timeout that no longer applies.
davidor
force-pushed
the
async-redis-worker
branch
from
66c0f7c
to
d5201ca
Compare
|
I addressed your comments @hallelujah . Thanks for the review. |
This PR continues the work started in #77
It adds a
Workerclass that processes jobs using the async storage introduced in #77Notice that the base branch of this PR is the integration branch
async, notmaster.Note: CircleCI fails because the CI image contains a ruby version that is not compatible with this feature (async lib requires ruby >= 2.2.7).