Shifted some responsibilities around

[emmanuel]

Hi,

Thanks for the great Resque plugin.

I did a little refactoring in my fork which I humbly offer for your consideration.

-- Emmanuel

[lmarlow]

I do like pulling the timestamps out of the metadata object... I wanted to do that initially, but there was no hook in resque at the time to track when a job was enqueued. I believe it could be achieved with the after_enqueue hook now. However, I think the dependency is backwards. The meta plugin should just be a generic databag and we should create a timestamp plugin that extends meta plugin, ala resque-result (https://github.com/lmarlow/resque-result/blob/master/lib/resque/plugins/result.rb#L33).

What is the reasoning for moving the save and retrieve logic into Metadata? Is it stylistic or do you feel it belongs there? I'm undecided and lean towards leaving it where it is mainly due to inertia :)

[emmanuel]

I think you're absolutely right. I have Meta extending Timestamps right now to preserve the existing interface for as long as possible. Well, at least until I talked to you about such a radical change. If you're up for separating them, I'll flip the include around.

Moving the get/store interface onto Metadata makes more sense to me because of coherency: those methods are getting and storing Metadata instances, and should therefore be class methods on Metadata.

I completely agree about metadata being just a data bag, and all contents should get externalized. I was thinking today that the meta_id param is really job_id, in the sense that it's being used to identify this particular job instance on the queue, which secondarily allows the metadata to be associated with it. In this view, meta_id/job_id is the very essence of resque-meta, and might even be it's own thing (ie., Resque::Plugins::Meta might include Resque::Plugins::JobId).

Other changes I'd like to make: moving Metadata elsewhere in the class hierarchy, probably to Resque::Job::Metadata, and renaming the plugin to resque-metadata (Resque::Plugins::Metadata).

But this is a lot of talk on my part. In the meantime, I wasn't running tests often enough, and I've broken things. Definitely do not pull yet. I will clean things up and get tests passing again, but it may be a couple of days because I'm going out of town.

[emmanuel]

I went ahead and pulled out a JobIdentity plugin, which makes a total of 3 (JobIdentity, Meta, and Timestamps).

I was wrong about moving Metadata in the class hierarchy, it definitely should stay in Resque::Plugins::Meta. Not sure why that seemed like a good idea earlier.

These are pretty sweeping changes. Please let me know what you think.

[emmanuel]

A couple more fairly large changes, but I think quite simplifying: I moved #enqueue from Meta to JobIdentity and added before_enqueue hooks to facilitate creating the metadata before the job is enqueued.

This is about the end of my ideas for the plugin; I think this latest iteration is pretty coherent and modular. I hope you agree.

[emmanuel]

Oh, and I should mention that all tests are passing (after some updating) and that this might be three (small) resque plugins now. Also, I'm going to see if there's any interest in pulling JobIdentity (or something like it) upstream into Resque itself. I think it would make sense as an addition to the core, but we'll see.

[emmanuel]

No dice on getting this merged in upstream. I opened an issue against Resque (defunkt/resque#244), but then I noticed that this was discussed long ago: defunkt/resque#6.

[emmanuel]

OK, there was some more opportunity for improving coherence by moving left-behind methods from Metadata into Timestamps::Metadata. This finishes the job of extracting Timestamps from Metadata.

I also changed the Metadata interface to make it a little bit like HashWithIndifferentAccess (convert lookup keys with #to_s).

I haven't heard any feedback in a bit. What do you think of all these changes? Are you interested in merging them in?

[lmarlow]

I think there are some good changes in here. I'm still not sure about moving store and get into the metadata object itself. I also wonder whether the timestamp stuff should be pulled into a separate plugin that depends on resque-meta or whether it just an additional file that's included with resque-meta like you've done.

Sorry I haven't given feedback sooner, just haven't had time recently to give it my full attention.

[emmanuel]

The reason for putting the get/load/store methods in Metadata: coherence! Those methods deal with Metadata instances, and the alternative is to put the responsibility for get/load/store of Metadata instances on Job classes (via Meta). Ideally Job classes should have no knowledge of how to get/load/store Metadata instances, but I conceded by leaving the Meta#get_meta method. Making the Job classes responsible for getting/loading/storing Metadata instances bleeds responsibilities; more appropriate would be to remove Meta#get_meta and let clients retrieve the Metadata instance using Metadata.get.

In my fork, the responsibilities are clear:

• JobIdentity manages generating and inject the job_id arg (as well as adding the before_enqueue hook)
• Meta manages creating a blank Metadata for every enqueued Job
• Metadata instances are the actual data bags that store Job instance data
• Timestamps adds enqueued, started, finished/failed timestamps to Metadata (and some conveniences)

As far as separate plugins go: I think it makes sense to continue packaging JobIdentity, Meta and Timestamps together as one gem (resque-meta), but let them remain as separate plugins within the one gem: clients simply extend their job classes with the module they want. Since Meta includes JobIdentity and Timestamps includes Meta, you only have to extend one module to get everything.

[emmanuel]

Hi @lmarlow!

It's been a while since last I heard anything about this. Do you have any interest in merging in these improvements?

[lmarlow]

I have a branch with your changes pulled in and would like to go over
it before I push it up. Sorry for the delay, it is on my list of
things to get to, though.

On Mon, Jul 25, 2011 at 2:32 AM, emmanuel
[email protected]
wrote:

Hi @lmarlow!

It's been a while since last I heard anything about this. Do you have any interest in merging in these improvements?

Reply to this email directly or view it on GitHub:
#4 (comment)

[emmanuel]

No problem, I just was looking through my repos and realized that I didn't know your thoughts about these changes. Let me know if there's anything you're concerned about once you get into it and take a look.

[millisami]

Any updates?

Does it work with resque v1.19.0 along with resque-status that changes the job klass's def self.perform method to instance method def perform ?