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

Upgrade to Jesque 1.3.0 #24

Closed
xnickmx opened this issue Feb 16, 2013 · 6 comments
Closed

Upgrade to Jesque 1.3.0 #24

xnickmx opened this issue Feb 16, 2013 · 6 comments

Comments

@xnickmx
Copy link
Contributor

xnickmx commented Feb 16, 2013

I would love to modify the Grails Jesque plugin to expose some new features in Jesque 1.3.0. I attempted to upgrade the plugin myself, but an exception was thrown when the only change I made was upgrading the Jesque library.

Problem

I changed the version of Jesque in BuildConfig from 1.2.0 to 1.3.0.
Then I tried to run my Grails app that uses the Jesque plugin, and I got this exception:

java.lang.UnsupportedOperationException
at java.util.Collections$UnmodifiableMap.put(Collections.java:1342)
at grails.plugin.jesque.GrailsWorkerImpl.addJobType(GrailsWorkerImpl.groovy:47)
at net.greghaines.jesque.worker.WorkerImpl.setJobTypes(WorkerImpl.java:410)
at net.greghaines.jesque.worker.WorkerImpl.<init>(WorkerImpl.java:178)
at grails.plugin.jesque.GrailsWorkerImpl.<init>(GrailsWorkerImpl.groovy:24)
at grails.plugin.jesque.JesqueService.startWorker(JesqueService.groovy:115)
at grails.plugin.jesque.JesqueService$_startWorkersFromConfig_closure4_closure6.doCall(JesqueService.groovy:165)
at grails.plugin.jesque.JesqueService$_startWorkersFromConfig_closure4.doCall(JesqueService.groovy:164)
at grails.plugin.jesque.JesqueService.startWorkersFromConfig(JesqueService.groovy:159)
at JesqueGrailsPlugin$_closure5.doCall(JesqueGrailsPlugin.groovy:162)
at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:334)
at java.util.concurrent.FutureTask.run(FutureTask.java:166)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1110)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:603)
at java.lang.Thread.run(Thread.java:722)

I think I have diagnosed the problem, but I don't know what the fix should be.

Diagnosis

The key changes appears to be WorkerImpl in v 1.3.0 now calls addJobType, which it didn't do before. This ends up calling GrailsWorkerImpl.addJobType, which attempts to modify an unmodifiable map, which throws the exception.

Details:

  • It seems that in Jesque 1.3.0, the WorkerImpl's constructor is now calling the addJobType() method. It didn't do this in Jesque 1.2.0.
  • The GrailsWorkerImpl overrides this method with its own implementation.
  • The original implementation, in WorkerImpl, requires the jobType that is added to either implement Runnable or Callable.
  • Your implementation appears to be identical to the original, except it omits the Runnable/Callable requirement. This makes sense as the Job classes are POGOs.
  • Your implementation references "this.jobTypes"
  • The jobTypes member variable is declared private by WorkerImpl (and has always been private).
  • Therefore, "this.jobTypes" ends up calling the getJobTypes method.
  • The getJobTypes method returns an unmodifiable map.
  • In your implementation of addJobType, you call the put method on this unmodifiable map which results in the UnsupportedOperationException.

Has GrailsWorkerImpl.addJobType just not been getting called? It doesn't seem like this would have worked at any time in the past two years or so, which is as far back as I looked in the history for WorkerImpl. This map has been unmodifiable for a long time and the member variable has been private since the first commit.

Workaround

I did figure out a kind of ugly workaround.

  • Remove GrailsWorkerImpl.addJobType
  • Make all jobs implement Runnable, but with a run() method that only throws an UnsupportedOperationException, since we don't want this to actually ever get called and want to know if it does.
    • All jobs in the plugin (AutoWireJob, DomainJob, etc) implement Runnable
    • All jobs in my app

This did work. It has the added ugliness of worthless Runnable implementations on all of the jobs, but it seemed to work fine with Jesque 1.3.0.

Better Fix?

I am guessing you will have a more elegant, permanent fix for this.

If you can suggest something else to try, I will do it. Assuming it works, I can send a pull request to you with the upgrades.

@xnickmx
Copy link
Contributor Author

xnickmx commented Feb 21, 2013

I have been wondering if there is a way to make all of the job classes Runnable with Groovy metaprogramming. It seemed like it might be possible to do and would be attractive because it would be backward compatible.

So far, unfortunately, I have been unsuccessful. I haven't been able to get something to work that can be passed into WorkerImpl's constructor and will also have Runnable.class.isAssignableFrom(jobType) return true.

Have you had a chance to look into this issue at all yet?

@michaelcameron
Copy link
Owner

Sorry, I had some time off, and then took some time to get back into things. I'll be looking at this now.

@michaelcameron
Copy link
Owner

I think we just need to change the upstream Jesque project to allow the subclass to validate the jobType (gresrun/jesque#29). After that's approved, I'll release a version of grails-jesque.

Thanks for the research and attempts at changing, it made tracking down the issue a lot easier.

@michaelcameron
Copy link
Owner

I've published a 0.6.0-SNAPSHOT with jesque 1.3.1. I plan on publising a final 0.6.0 after we deploy in production, and potentially doing something for issue #23, hopefully in the next few days.

@xnickmx
Copy link
Contributor Author

xnickmx commented Mar 22, 2013

Thanks for getting this done. I submitted a pull request to get some new features into this plugin based on improvements in Jesque 1.3.0.

@michaelcameron
Copy link
Owner

I've merged that pull request and published it as 0.6.1

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