-
Notifications
You must be signed in to change notification settings - Fork 4
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
"Manual mode" #8
Comments
Interesting idea, do you have a use case for this kind of "dynamic" scheduling?
I agree, the gem is really simple at the moment and does it's job pretty well. I think we migrated away for the previous solution due to it's complexity and performance problems with the My main gripe with the current way of having the class level Having a different way to specify the recurrence, either via a class or instance method would give us control when we are evaluating the method. Doing that when the sidekiq server boots would, i believe, allow us to get rid of a few guards we have in our |
I see. That was the reason for delaying the cronline evaluation in ca994e3 ? Did that not fix all your issues? |
Oh, yay past me! I totally forgot about those changes 😄 |
One of my coworkers argued that using a middleware (+ the
repeat { }
configuration at class-level) was overly complicated, and that instead one could just have an instance methodreschedule
that is manually called insideperform
.For example:
Basically, with this, you don't even need the gem at all anymore, since you could simply call
self.class.perform_in
and be set. However, of course you'd still need a way to schedule jobs (or ensure their scheduledness) at boot time, and you would probably add some boilerplate around it, ending up at the gem solution after all.So, I'd generally argue that the perceived simplicity gain would quickly fade away, as sidekiq-repeat itself is already super simple.
The one benefit I do see with this is the additional level of control it provides you. You could easily change your reschedule pattern according to criteria you choose yourself (e.g. previous run duration), or you could even choose to not reschedule at all. Do you think this would be a good feature to have?
Changes required:
reschedule { cron_line_or_ice_cube_expr }
method that redirects to class-levelreschedule
reschedule
would need an optional cronline, and only if that isnil
would ask for the the class configurationcronline
Not sure if this really makes sense, but obviously I'm trying to find a way to sell sidekiq-repeat :)
The text was updated successfully, but these errors were encountered: