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

Allow jobs to declare theirs schedule #715

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

and9000
Copy link
Contributor

@and9000 and9000 commented Jul 11, 2021

Hello,

I've implemented the possibility for the job to declare it's schedule so it's self contained.

@and9000
Copy link
Contributor Author

and9000 commented Jul 26, 2021

@jeremy sorry for pinging you directly, do you have any though about this? Thanks!

Btw as we are here 😄 I work for a company (Nozomi Networks - OT & IoT Security and Visibility) which would like to contribute and maintain this project (and moving on to resque itselt) as we heavily use ruby and rely on resque. Do you have any suggestion for us on how we could help?

Thanks!

@iloveitaly
Copy link
Contributor

@and9000 Would love your help maintaining this!

The first thing we'll need to do is get CI working again. Could you submit a separate PR to move CI to GH actions? Here's what we did for the core resque gem: https://github.com/resque/resque/blob/master/.github/workflows/ci.yml

@and9000
Copy link
Contributor Author

and9000 commented Sep 13, 2021

Sure, I'll start working on CI 👍

Copy link
Contributor

@iloveitaly iloveitaly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome idea! I love the idea of (optionally) grouping the scheduling logic with the class definition—makes it much more clear to the reader whats going on.

Couple thoughts about the specific implementation, but I'd love to get this change in.

def queue(value = nil)
return @queue ||= nil if value.nil?
@queue = value
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard Resque convention is to use @queue without a class method. I think we should continue to use that convention if we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Fixed.

rescue NameError
log! "Can't load file #{file}"
end
load_schedule_job(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to run load_schedule_job when the schedule is set up on the class? If we used a resque_schedule method on the class-level as suggested below, do you think we could eliminate the auto-load logic from the PR and instead run load_schedule_job as part of resque_schedule.

Autoloading and scheduling on the class-level feel like two different concerns to me, and it's not clear why we should couple them in this case.

Additionally, autoloading feels like something that makes sense on the core resque level as opposed to the scheduler plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved schedule logic into class method resque_schedule.

I'm still stuck thinking on where the autoloading feature should be: in principle yes it shouldn't be here but on the application level (not even resque) as the app is responsible to load stuff. But this mean we should add a big warning "in order to self-schedule-jobs to work please remember to require your jobs". But this is exactly what auto_load is doing so my doubt is: why don't make that requirement explicit?

end

module ClassMethods
def cron(value = nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If feels like it could be cleaner to have a single class method resque_schedule which has a bunch of optional args (cron:, every:, etc). I don't love the idea of adding a bunch of methods that aren't clearly ties to resque scheduler (i.e. description could easily be a method users of this gem have defined on their class).

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes code looks cleaner this way 😄

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

Successfully merging this pull request may close these issues.

2 participants