Skip to content
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

Open
maltoe opened this issue Nov 16, 2018 · 3 comments
Open

"Manual mode" #8

maltoe opened this issue Nov 16, 2018 · 3 comments

Comments

@maltoe
Copy link
Contributor

maltoe commented Nov 16, 2018

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 method reschedule that is manually called inside perform.

For example:

class TestJob
  include Sidekiq::Worker
  include Sidekiq::Repeat::Repeatable

  def perform
     # ... do some work
  ensure
    reschedule 1.hour # or cron pattern, or whatever
  end
end

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:

  • A new instance level reschedule { cron_line_or_ice_cube_expr } method that redirects to class-level reschedule
  • Class-level reschedule would need an optional cronline, and only if that is nil would ask for the the class configuration cronline

Not sure if this really makes sense, but obviously I'm trying to find a way to sell sidekiq-repeat :)

@dsander
Copy link
Member

dsander commented Nov 19, 2018

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?

Interesting idea, do you have a use case for this kind of "dynamic" scheduling?

So, I'd generally argue that the perceived simplicity gain would quickly fade away, as sidekiq-repeat itself is already super simple.

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 ice_cube gem.

My main gripe with the current way of having the class level repeat method (I can never remember the proper name for these "hook" methods) is that it is executed whenever the file is required. Depending on when the (manual) require is done the rails application might not be booted completely yet, or it is required when rake db:create is called and trying to access the database will cause exceptions.

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 repeat methods.

@maltoe
Copy link
Contributor Author

maltoe commented Nov 19, 2018

I see. That was the reason for delaying the cronline evaluation in ca994e3 ? Did that not fix all your issues?

@dsander
Copy link
Member

dsander commented Nov 19, 2018

Oh, yay past me! I totally forgot about those changes 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants