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

Projects
None yet
4 participants
@vbekiaris
Copy link
Contributor

commented Apr 4, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2016

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 vbekiaris force-pushed the vbekiaris:maintenance-3.6.x-7854 branch from 269ee94 to 057ca88 Apr 4, 2016

@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2016

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)) {

This comment has been minimized.

Copy link
@serkan-ozal

serkan-ozal Apr 4, 2016

Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2016

👍 minor comment

@vbekiaris vbekiaris force-pushed the vbekiaris:maintenance-3.6.x-7854 branch from 057ca88 to a3e0b64 Apr 4, 2016

@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2016

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2016

@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

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2016

ok, fair enough.

Fix for #7854: introduced JCacheDetector to provide single point of J…
…Cache detection. Also, we now test for additional classes that guarantee JCache 1.0.0 is on the classpath.

@vbekiaris vbekiaris force-pushed the vbekiaris:maintenance-3.6.x-7854 branch from a3e0b64 to 8ca344d Apr 5, 2016

@vbekiaris

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2016

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

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2016

👍

1 similar comment
@serkan-ozal

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2016

👍

@serkan-ozal serkan-ozal merged commit 2ef0bc6 into hazelcast:maintenance-3.x Apr 5, 2016

1 check passed

default 10875 tests run, 39 skipped, 0 failed.
Details

@vbekiaris vbekiaris deleted the vbekiaris:maintenance-3.6.x-7854 branch Apr 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.