Add OSGi support

[cschneider]

I added the necessary plugins and config to create an OSGi bundle during the build. This should not influence usage outside of OSGi.

You can install in OSGi by downloading and installing apache karaf 4.1.1 and issue these commands:
install -s mvn:org.threeten/threeten-extra/1.2
install -s mvn:de.jollyday/jollyday/0.5.3-SNAPSHOT

The bundles should start without issues.

[coveralls]

Coverage remained the same at 84.785% when pulling 5a0aa69 on cschneider:OSGi-support into 4579dd7 on svendiedrichsen:master.

[svendiedrichsen]

Thank you very much, Christian. The API relies heavily on loading resource files from classloader which I have thought to be some problem in OSGI. There is a branch at https://github.com/cnoelle/jollyday where someone did some OSGIcation but IMHO did not square the fact that in an JEE environment you should access resources from classloader by using the current threads CL.
Does loading resources from CL by using the currents thread CL also work in OSGI by now?

[cschneider]

I don't think this is already tackled. I asked my colleague Andrei to test in OSGi and give some feedback. He is already using the code in OSGi so there at least seems to be a workaround. I guess he is also setting the ContextClassloader. Btw. if you need to load resources then it is a good idea to let the user submit a Classloader in the call. This then works nicely for OSGi as well as JEE.

[ashakirin]

Hi,
Thread context class loader works in OSGi.
I need to wrap HolidayManager call in following construction:

    try {
        origClassLoader = Thread.currentThread().getContextClassLoader();
        Thread.currentThread().setContextClassLoader(HolidayManager.class.getClassLoader());
        holidayManager = HolidayManager.getInstance(HolidayCalendar.GERMANY);
    } finally {
        Thread.currentThread().setContextClassLoader(origClassLoader);
    }

[cschneider]

Btw. Why do you define all the impl class names in jolliday.properties? I think you would have a lot less issues by just defining the default impl classes in java and letting the user set his new impls using a Java API.

For example I could imagine a builder pattern like:
manager = new HolidayManagerBuilder().build();
and for example:
manager = new HolidayManagerBuilder().christianHoliday(new MyChristianHolidayImpl()).build();
to override a specific impl class.

Such an approach would also give the user all the flexibility and is compatible to any deployment environment as you do not have to rely on specific setup of classloaders.

[svendiedrichsen]

The classloaders primary usage is not loading the properties files. Those properties can be defaulted in code but loading the holiday data files as resources from classpath needs the appropriate CL.

[cschneider]

I agree for the holiday data files. The context classloader with fallback to your own classloader is a good bet there.

I just looked a bit into the code that loads the classes as well as the xml file.

I found a possible issue. You have two ways of loading classes:
de.jollyday.caching.HolidayManagerValueHandler.instantiateManagerImpl uses classLoadingUtil.loadClass
which has a fallback to Class.forname which in OSGi uses the current bundle. I think this is one is ok.

On the other hand de.jollyday.datasource.ConfigurationDataSourceManager.instantiateDataSource uses classLoadingUtil.getClassloader().loadClass which only uses the context classloader. So this will not work if the configuration.datasource.impl should be loaded from your jollydays bundle and the user set the context classloader to its own bundle classloader. I propose to also use classLoadingUtil.loadClass here.

In general though I really recommend to migrate away from loading any impl classes yourself. You would save yourself lots of trouble. The really best advice for OSGi compatibility and I guess also application server compatiblity is to not use a Classloader in your code.

I will do some tests in OSGi and give feedback if I find issues.

[cschneider]

I created a small test project https://github.com/cschneider/jollydaystest
Using this code jollydays works fine with defaults in OSGi.

One issue is that if I leave out setting the context classloader I get a NPE.
https://github.com/cschneider/jollydaystest/blob/master/src/main/java/net/lr/jollydays/HolidaysTest.java#L17

I think this is not ideal. I also think that it would not work to use the classloader of my own bundle if I wanted to override settings as then jollydays would not find its own resources anymore.

[coveralls]

Coverage decreased (-0.03%) to 84.75% when pulling 3a250f2 on cschneider:OSGi-support into 4579dd7 on svendiedrichsen:master.

[cschneider]

I fixed the class and resource loading. So by default it is not necessary anymore to set the context classloader. I think the code is working fine now in OSGi.

[ashakirin]

Hi Christian,

I have tested version from your branch, the HolidayManager works in OSGi in all alternatives:
HolidayManager mgr1 = HolidayManager.getInstance();
HolidayManager mgr2 = HolidayManager.getInstance(HolidayCalendar.GERMANY);
ManagerParameter params = ManagerParameters.create(Locale.GERMANY);
HolidayManager mgr3 = HolidayManager.getInstance(params);

Only the thing I noticed: additional bundle dependency threeten-extra is always required now (in comparison with wrapped version). Because this dependency is used only in some CalendarUtils methods referencing CopticChronology and JulianChronology, perhaps it makes sence to set threeten-extra dependency as optional in OSGi, WDYT?

Regards,
Andrei.

[cschneider]

We can make it optional but as threeten-extra is a already a bundle and has not further deps I think it is quite ok to have it.

[cschneider]

@svendiedrichsen What do you think about the current state?

[ashakirin]

@cschneider: yep, threeten-extra is a bundle, but user have to care about adding it (that wasn't always necessary in wrapped version). Not sure what is best option here, let pick up additional opinion from jolly team.

[svendiedrichsen]

Sorry for the long delay. I have been on vacation for some time and pretty busy after that. Sorry.

Concerning classloading I agree that it would be better to not load the classes from a properties file and I will work on this. The builder seems to be a good option there.

Concerning the dependency I think it is unavoidable as the needed Chronologies are not part of the JDKs time api. It seems to be usable in an OSGI environment as the threeten-extra lib is OSGI capable.