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

[BACKPORT] JCache detection improvement #7899

Merged

Conversation

vbekiaris
Copy link
Collaborator

This PR fixes #7854 by introducing a separate utility class com.hazelcast.cache.impl.JCacheDetector which handles JCache detection; specifically we make sure we locate a number of classes which are available since JCache 1.0.0-PFD but not in previous versions which caused NoClassDefFound exception as shown in #7810 (therefore when JCache 0.9 or 0.5 is in the classpath, Hazelcast will consider JCache is not available).

@noctarius
Copy link
Contributor

I think it should be extended to prevent 1.0.0-PFD from being used: http://stackoverflow.com/questions/36387190/error-in-jcache-implementation-with-hazelcast-completeconfiguration-class-not-fo/36387326#36387326

Maybe give it an error which states to use 1.0.0 final version!

@vbekiaris
Copy link
Collaborator Author

Amended to require JCache cache-api 1.0.0. In case class javax.cache.Caching is detected in the classpath but one or more of other required JCache API classes are missing, a warning is logged to indicate that JCache 1.0.0 is required and consider JCache is missing.

@jerrinot jerrinot added this to the 3.6.3 milestone Apr 4, 2016
@vbekiaris vbekiaris changed the title Fix for issue #7854 - JCache detection [BACKPORT] JCache detection improvement Apr 4, 2016
}
for (String className : JCACHE_ADDITIONAL_REQUIRED_CLASSES) {
if (!ClassLoaderUtil.isClassAvailable(classLoader, className)) {
if (HINT_LOGGED.compareAndSet(false, true)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this logging logic? Because the logging check is global but availability check is classloader based.

I mean, assume that there are at least two different classloader and none of them knows jcache (can't detect jcache classes). In this case, this log message is displayed for only one of them, not for both of them.

Therefore, I think, logging should be responsibility of the caller of JCacheDetector.

@serkan-ozal
Copy link
Contributor

👍 minor comment

@vbekiaris
Copy link
Collaborator Author

Thanks Serkan, I have amended the code so that the caller can pass its own logger which will be used to log the warning in case JCache < 1.0.0 is located on the classpath.

@serkan-ozal
Copy link
Contributor

I have discussed with @vbekiaris and to me using explicit version number in the code base (V1_0_0) seems redundant and confusing. Because;

  • If a new jcache version is published like 2.0.0 and there is no change for the checked classes (still we will check javax.cache.Caching, javax.cache.integration.CacheLoaderException, ...). In this case for that 2.0.0 version we will still use 1_0_0 enum (if we don't update the code as mentioned above) but in fact it is not.
  • I don't expect very often published new jcache versions but my point is that we shouldn't rely on explicit version numbers in the code base. Imagine that after 1 year later, jcache-api 2.0.0 published and will we update all callers in the code base like if (checkJCacheVersion(classloader, logger) == V1_0_0) || if (checkJCacheVersion(classloader, logger) == V2_0_0)?. At the moment, we just use == V1_0_0 for all callers and it is not different for me by checking as true/false.

Therefore, I prefer simple true/false logic which is fair enough at the moment.

My offer is

public static boolean isJcacheAvailable(ClassLoader classLoader) {
    return isJcacheAvailable(classLoader, null);
}

public static boolean isJcacheAvailable(ClassLoader classLoader, ILogger logger) {
    if (!ClassLoaderUtil.isClassAvailable(classLoader, JCACHE_CACHING_CLASSNAME)) {
        // no cache-api jar in the classpath
        return false;
    }
    for (String className : JCACHE_ADDITIONAL_REQUIRED_CLASSES) {
        if (!ClassLoaderUtil.isClassAvailable(classLoader, className)) {
            if (logger != null) {
                logger.warning("An outdated version of JCache API was located in the classpath, please use newer versions of "
                      + "JCache API rather than 1.0.0-PFD or 0.x versions.");
            }
            return false;
        }
    }
    return true; 
}

@jerrinot
Copy link
Contributor

jerrinot commented Apr 5, 2016

@serkan-ozal: what's the semantic of the true?
Does it mean "some version of JCache was found" ? or is it rather "JCache 1.0 Final or newer was found" ?

Your example could be improved like this:

JCacheVersion version = checkJCacheVersion(classloader, logger);
if (version.isAtLeastVersion(V1_0_0) {
  [...]
}

Is it still ugly?

@serkan-ozal
Copy link
Contributor

@jerrinot

@serkan-ozal: what's the semantic of the true?
Does it mean "some version of JCache was found" ? or is it rather "JCache 1.0 Final or newer was found" ?

true means that JCache 1.0 Final or newer was found. Its meaning might be changed in the future with the new jcache versions. I mean, it might mean basic jcache functionality based on jcache 1.0 final in the future.

Your example could be improved like this:
JCacheVersion version = checkJCacheVersion(classloader, logger);
if (version.isAtLeastVersion(V1_0_0) {
[...]
}
Is it still ugly?

It is not ugly and we had discussed very similar ideas with @vbekiaris yesterday. But my point is that the callers of JCacheDetector is only interested in same functionalities at the moment. So using explicit version in the code base seems has no added value for me at the moment. In the future, with never versions of jcache, we can use explicit version checking for some new functionalities provided by hazelcast regarding to new jcache api if there is. But at the moment, I don't see any added value of using explicit version in the code base.

@jerrinot
Copy link
Contributor

jerrinot commented Apr 5, 2016

ok, fair enough.

…oint of JCache detection. Also, we now test for additional classes that guarantee JCache 1.0.0 is on the classpath.
@vbekiaris
Copy link
Collaborator Author

Thank you all for your contributions, code was updated according to comments.

@jerrinot
Copy link
Contributor

jerrinot commented Apr 5, 2016

👍

1 similar comment
@serkan-ozal
Copy link
Contributor

👍

@serkan-ozal serkan-ozal merged commit 2ef0bc6 into hazelcast:maintenance-3.x Apr 5, 2016
@vbekiaris vbekiaris deleted the maintenance-3.6.x-7854 branch April 5, 2016 15:32
@mmedenjak mmedenjak added the Source: Internal PR or issue was opened by an employee label Apr 13, 2020
@Dineshseervi
Copy link

Dineshseervi commented Sep 17, 2020

This issue is still there .I have javax.cache (1.0.0) as dependency in my project and using Hazelcast (3.11).
On debug in my project i found javax.cache class are there in Parallelwebappclass loader but not in UrlClassLoader.
Now Hazelcast have code which will look in ClassLoader passed (if not found) then it looks in (Current Thread classLoader which is Parallelwebappclass) so it found javax.cache classes are present . After that Hazelcast class call (new ClientExceptions(jcacheAvailable)) where ClientExceptions is using UrlClassLoader which don't have javax.cache packed classed loaded so i am getting ERROR.

-------Error message----

Caused by: java.lang.NoClassDefFoundError: javax/cache/CacheException
	at com.hazelcast.client.impl.protocol.ClientExceptions.<init>(ClientExceptions.java:108)
	at com.hazelcast.client.impl.ClientEngineImpl.initClientExceptionFactory(ClientEngineImpl.java:155)
	at com.hazelcast.client.impl.ClientEngineImpl.<init>(ClientEngineImpl.java:148)
	at com.hazelcast.instance.Node.<init>(Node.java:227)
	at com.hazelcast.instance.HazelcastInstanceImpl.createNode(HazelcastInstanceImpl.java:156)
	at com.hazelcast.instance.HazelcastInstanceImpl.<init>(HazelcastInstanceImpl.java:126)
	at com.hazelcast.instance.HazelcastInstanceFactory.constructHazelcastInstance(HazelcastInstanceFactory.java:202)
	at com.hazelcast.instance.HazelcastInstanceFactory.getOrCreateHazelcastInstance(HazelcastInstanceFactory.java:111)
	at com.hazelcast.core.Hazelcast.getOrCreateHazelcastInstance(Hazelcast.java:165)

@jerrinot
Copy link
Contributor

@Dineshseervi: can you shed some light into your setup? Are you using any application container/server? Is JCache loaded by a different classloader than Hazelcast?

@Dineshseervi
Copy link

@jerrinot
I am using a spring maven based application where i have Hazelcast dependency and javax.cache (1.0.0) as dependency in POM.
When i try to deploy my application (as WAR) on tomcat server i am facing this issue.

@jerrinot
Copy link
Contributor

@Dineshseervi thanks for update!
If both Hazelcast and JCache are dependencies in your app POM then they should be in the same classloading hiearchy. Spring + Tomcat is a common combination, I am surprised you are running into the issue. What's your Spring&Tomcat version?

@mmedenjak
Copy link
Contributor

@Dineshseervi any updates on the issue? Can you compress your project and open a new issue with it so we follow up on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants