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
2.0 consideration: make job args a hash? #912
Comments
|
I think that forcing people to make it a hash is a bad idea. This situation is no different than any other class, really: if you have a bunch of args, you should consider making them a hash (or 2.0 keyword args) so that this kind of thing doesn't happen to you. That said, I'm open to something that alleviates this pain, i'm just not 100% sure that enforcing a signature is the way to do it. Anyone else? |
|
I'm not entirely sold on the idea, either; not exactly sure the right thing here. Though any kind of jobs that run on a queue are different than standard classes in that they can be executed at an arbitrary time from their creation time (queue might be backed up, you might have some kind of scheduler that enqueues the job to run at some time in the future), and they run on another machine, so you don't have the strong code consistency guarantees you have in other applications (where you might deploy a change and now your app runs all the new code -- with Resque, I might deploy a change to my API but background workers might not have restarted and are still running the old code (or half might have restarted, the other half might not have), but I can't even enqueue the new signature at that point). It's not that terrible to deal with, so I wouldn't think this be a high priority, but it can be frustrating. |
|
I'm curious about others' thoughts here, but past that you can just close it out then. |
|
I think requiring any kind of arg format in Resque is a bad idea. In your particular case you might want to take a page from JavaScript's book. self.perform(*args)
arg1, arg3, arg4 = if args.length == 4
args[0,2,3]
else
args
end
endThen once arg2 is safely deprecated you can remove this. That should achieve zero-downtime like you want. |
|
I agree with @steveklabnik - it's no different than anything else. Any time you write a method with multiple arguments, you have a dependency on their presence and order. Since the vast majority of my resque jobs currently take a single integer arg, I wouldn't want to see a different (or any, really) signature enforced. |
|
Closing until proven guilty. ;) |
|
Thanks for such fast feedback, everyone. |
|
@steveklabnik, is resque@librelist.com working and used? I recall awhile back I had asked hone about a mailing list that had appeared to be deserted. If so, I'll subscribe and post my various thoughts there instead of issues. |
|
It is. I, in fact, just posted a thread on it pointing people to this ticket. ;) |
|
Fan-freaking-tastic. Subscribed, thanks. |
|
Most of the time my jobs really just have 1 arg like id in my use cases, so forcing a hash seems weird. But how about making example jobs in the Readme take hashes, so new users are pointed towards considering their use as arguments? |
We have a job with a perform method like
self.perform(arg1, arg2, arg3, arg4)that no longer needs arg2; removing that arg requires us creating a new job with different arguments, moving ourResque.enqueuecode over to it, and then getting rid of the old job. These jobs get enqueued extremely frequently, so this is the best I can come up with to have zero downtime. I don't want to suffer through any job failures due to ArgumentErrors.There are some alternatives out there, where I could have had
self.perform(opts = {})and had my job take a hash, then I wouldn't be in this situation.Though, I feel like this is one of those situations where I had enough rope to hang myself, so by "implementing it badly" without using a hash in the first place, I have to suffer through a migration of the job to update its signature.
How do other people handle this? What are your thoughts @steveklabnik on making job args be a hash for 2.0 anyway so no one has to deal with this?
The text was updated successfully, but these errors were encountered: