WASdev / standards.jsr352.jbatch

Home of 'jbatch', a compatible implementation of the Jakarta Batch specification (and the former Reference Implementation for the JSR 352, Batch Applications for the Java Platform specification).
Other
21 stars 18 forks source link

Some changes required to enable overriding of the Persistence Service #17

Closed smillidge closed 9 years ago

smillidge commented 9 years ago

Scott,

These are some changes I needed to make to override the persistence service with one in a Payara (GlassFish module).

First change is using the ThreadContextClassLoader within ServicesManagerImpl rather than just Class.forName

Second change is exporting more of the APIs from the Manifest as GlassFish is OSGI based I couldn't use the container classes without exporting them.

Third change is to the OSGI bundle names, I changed them to reflect the new maven coordinates. I can change this back if required.

With these changes I have successfully managed to completely override the provided persistence service with a custom one.

Steve

scottkurz commented 9 years ago

Steve,

Thanks for contributing this.

As far as the classloading, if we need to use ThreadContextClassLoader at all, I'm wondering if we can simply always use ThreadContextClassLoader, rather than failing over from current classloader to TCCL.

Would just like to keep things as simple as possible. Any chance you could give this a try?

I'm hesitant to add the additional packages for OSGI export, though. I'd like to understand what the need is for this a bit more. E.g. I don't want to just export the entire persistent service so it can be subclassed. OTOH, I could imagine how the com.ibm.jbatch.container.status pkg, say, might need to be part of the SPI. Basically I'd like to take it one interface at a time... so could you please explain which ones are needed?

scottkurz commented 9 years ago

If you want to "explain" by just showing a (possibly stubbed) impl class that's probably all I'll need to start.

smillidge commented 9 years ago

It just so happens that I have such a class (work in progress)

https://github.com/payara/Payara/blob/master/appserver/batch/hazelcast-jbatch-store/src/main/java/fish/payara/jbatch/persistence/hazelcast/HazelcastPersistenceService.java

Which is going to use Hazelcast as an internal store in Payara.

smillidge commented 9 years ago

I will try out just going straight to the TCCL I imagine it will work fine.

scottkurz commented 9 years ago

I'm going to leave this one for 1.0.2. Will work on sending 1.0.1. over to Glassfish tomorrow. Note I added a System property override in 329a56d, as a further override on top of the SPI override you (Steve) added. Also added some constants to work with the configurable services a bit more nicely programmatically.

You can see that I didn't "expose" persistence service in this list of constants. That just reflects the fact that we know it's not ready just yet... doesn't mean I don't want to get to that point in 1.0.2 say.

smillidge commented 9 years ago

Hi Scott

I tried just the TCCL and this fails in GlassFish for the services in the batch jar. We need the Class.forName and then the fall back to the TCCL .

Steve

smillidge commented 9 years ago

Hi Scott,

https://github.com/WASdev/standards.jsr352.jbatch/commit/329a56d55236ddc818e15d8965deb08e7f847dce misses the ThreadContextClassLoader fall back in _loadService(). This is essential to load a service from a different jar in an application server environment.

scottkurz commented 9 years ago

Steve,

OK we need to be able to try both.

Can we come up with a justification based on general Glassfish architecture/practice for preferring one versus the other? (And if we need to think more I'm fine saving this for 1.0.2).

smillidge commented 9 years ago

I've tried both ways.

Class.forName on its own fails to load services defined in other jars within GlassFish the TCCL on its own fails to load internal JBatch services within the same jar.

I think the code below is just a belt and braces approach.

Class cls; try { cls = Class.forName(className); } catch (ClassNotFoundException cnfe) { cls = Thread.currentThread().getContextClassLoader().loadClass(className); }

scottkurz commented 9 years ago

Right, I can see it's nice to have both options, I was just asking which to try first.

The way you coded it doesn't allow a TCCL override of a class bundled in the jbatch 'container' bundle, which seems fine by me.

Just giving a nod to the thought there might be some Glassfish precedent here I wasn't familiar with...

But then again this whole extension mechanism is very jbatch-specific, and your approach is more consistent with older versions.

Ok, probably overthought this... unless you disagree I'll commit this change too.

Thanks

scottkurz commented 9 years ago

Steve,

Though I committed the part trying the TCCL as a 2nd option, I plan to wait on the rest (exporting the new packages needed to implement the persistence service) until the new year. Just wanted to give you a heads-up on what to expect.
Thanks, Scott

smillidge commented 9 years ago

Hi Scott,

No problem. If you want to test 1.0.1 in "GlassFish" we have incorporated the latest 1.0.1 snapshot build in our Payara pre-release builds here http://www.payara.co.uk/upstream_builds.

Any idea when 1.0.1 will be pushed to maven central?

Steve