use enqueue instead of retry_job to reenqueue jobs #32
Conversation
GustavoCaso
force-pushed
the
do-not-use-retry-job-to-reenqueue-job
branch
from
ea7c15f to
0b226fb
Compare
GustavoCaso
force-pushed
the
do-not-use-retry-job-to-reenqueue-job
branch
from
0b226fb to
fc5657c
Compare
There was a problem hiding this comment.
I don't have a strong preference, but AFAIK @rafaelfranca recommended we use retry_job for re-enqueueing.
Semantically, I agree that this is not a "retry" but rather a "re-enqueue".
| @@ -150,7 +145,9 @@ def reenqueue_iteration_job | |||
| self.times_interrupted += 1 | |||
|
|
|||
| self.already_in_queue = true if respond_to?(:already_in_queue=) | |||
| retry_job | |||
|
|
|||
| enqueue unless @enqueued | |||
There was a problem hiding this comment.
You no-op enqueueing if @enqueued is set, but what about the rest of side-effects in that method? Why do they need to run if the job was already enqueued?
There was a problem hiding this comment.
Unless I'm missing something you need the defined?(@enqueued) otherwise you will get a Ruby warning.
|
I'm not against this change, if the goal if to make |
@kirs I wanted to see if we can remove the call for
retry_joband all the test will pass :)I wanted to remove the use of
retry_jobbecause I plan on overwrite it on core to increase the number of attempts from theRetrymodule, to make sure are not retrying more than they should.