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

Hibernate 5.2, Ehcache 3, JCache support #4454

Merged
merged 13 commits into from Nov 23, 2016

Conversation

@henri-tremblay
Copy link
Contributor

commented Nov 9, 2016

This is the current version on my work in Hibernate 5.2 and Ehcache 3.

It includes the following:

  • Migrate Hibernate to 5.2
  • Migrate Jackson hibernate module to be compatible with Hibernate 5
  • Migrate liquibase-hibernate to version 3.6 which support Hibernate 5
  • Migrate to Ehcache 3 by using JCache
  • Migrate Metrics for Ehcache 3 to use JCache metrics

What is not working:

  • Hazelcast can probably be configured with JCache to have something more similar to Ehcache. And get JCache Metrics

Questions I have:

  • CacheConfiguration used to remove metrics. That is weird because it was removing more than Ehcache metrics. So I don't know if it was buggy or should be moved to MetricsConfiguration
  • Hibernate has changed its JPA compliant naming format. For instance a FK is now named FKblablabla instead of FK_blablabla. We have to provide our own implementation to fix that

What is fun:

  • Caching metrics are now coming from JCache so could be used by any JCache implementation
  • Because of JCache, there should be much more common code between caching implementation
  • It's easier to switch to another caching framework

@henri-tremblay henri-tremblay changed the title Hibernate 5.2, Ehcache 3, JCache support Hibernate 5.2, Ehcache 3, JCache support DO NOT MERGE Nov 9, 2016

@cbornet

This comment has been minimized.

Copy link
Member

commented Nov 9, 2016

You should be able to remove JSR310PersistenceConverters.java since Hibernate5 supports JSR310 types out-of-the-box

@PierreBesson

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2016

The JCache metrics is a very nice addition.

@henri-tremblay

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2016

@cbornet Thanks. I will look at it.

Can someone look at why Hazelcast isn't happy? It seems to come from com.hazelcast:hazelcast-hibernate52 that isn't really 5.2 compatible.

@henri-tremblay

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2016

Oh, I've added a little script to generate a JHipster project from all the samples. So, if you want to see the current failures, just run that. Travis should also return the same result of course.

@@ -26,14 +26,16 @@
if (key.indexOf('web.rest') !== -1 || key.indexOf('service') !== -1) {
vm.servicesStats[key] = value;
}
if (key.indexOf('net.sf.ehcache.Cache') !== -1) {
});
angular.forEach(newValue.gauges, function (value, key) {

This comment has been minimized.

Copy link
@deepu105

deepu105 Nov 9, 2016

Member

Note to self: Need to update for NG2 version as well cc @sendilkumarn @wmarques

@deepu105

This comment has been minimized.

Copy link
Member

commented Nov 9, 2016

I will take a look at Hazelcast, but I wont have time untill weekend so if anybody wants to take a hit plz do so @jdubois do you have any contacts from hazelcast team?

@jdubois

This comment has been minimized.

Copy link
Member

commented Nov 9, 2016

@deepu105 I need to check, but I'm not too worried about Hazelcast + 2nd level cache for Hibernate.
My main worry is Hazelcast HTTP session clustering, which adds a lot of issues: if we remove this, I'm confident about the rest.

@deepu105 deepu105 referenced this pull request Nov 9, 2016
29 of 29 tasks complete
@mesutcelik

This comment has been minimized.

Copy link

commented Nov 9, 2016

Hi @henri-tremblay @cbornet @jdubois ,

I think you have hit the bug if you are using hibernate 5.2.3.Final. We already fixed the problem in v1.1.1

Can you please try hazelcast-hibernate52-1.1.1?

P.S: hazelcast-hibernate52-1.1.1 was just released so it might take a few hours to be in maven central if you are using maven.

import static com.codahale.metrics.MetricRegistry.*;

/**
* MetricSet retriving JCache specific JMX metrics for every configured caches

This comment has been minimized.

Copy link
@jdubois

jdubois Nov 9, 2016

Member

You need a dot at the end of the sentence, so it's correct JavaDoc

This comment has been minimized.

Copy link
@henri-tremblay

henri-tremblay Nov 14, 2016

Author Contributor

Done

try {
this.objectInstances = ManagementFactory.getPlatformMBeanServer().queryMBeans(ObjectName.getInstance(M_BEAN_COORDINATES), null);
} catch (MalformedObjectNameException e) {
log.error("Failed to retrieve JCache statistics", e);

This comment has been minimized.

Copy link
@jdubois

jdubois Nov 9, 2016

Member

Shouldn't this be re-thrown? That should make start up fail?

This comment has been minimized.

Copy link
@henri-tremblay

henri-tremblay Nov 14, 2016

Author Contributor

The exception shouldn't happen since I'm providing the query. But I did a bunch of refactoring to make the code simpler and I am now throwing an InternalError

private String toDashCase(String camelCase) {
return camelCase.replaceAll("(.)(\\p{Upper})", "$1-$2").toLowerCase();
}

This comment has been minimized.

Copy link
@jdubois

jdubois Nov 9, 2016

Member

You have an extra line here

This comment has been minimized.

Copy link
@henri-tremblay

henri-tremblay Nov 14, 2016

Author Contributor

Done

import java.util.Properties;

/**
* Special hibernate region factory that will convert a Spring URI (e.g. classpath:ehcache.xml) to a real URI (e.g. file://ehcache.xml)

This comment has been minimized.

Copy link
@jdubois

jdubois Nov 9, 2016

Member

dot missing at the end of the sentence, and Hibernate should have a capital H

This comment has been minimized.

Copy link
@henri-tremblay

henri-tremblay Nov 14, 2016

Author Contributor

Done

@henri-tremblay henri-tremblay force-pushed the henri-tremblay:issue-4195 branch from c93e729 to a6de3cf Nov 14, 2016

@jdubois

This comment has been minimized.

Copy link
Member

commented Nov 22, 2016

@henri-tremblay I would like to do a specific release for this, do you know when your PR will be ready?

@henri-tremblay henri-tremblay force-pushed the henri-tremblay:issue-4195 branch from cfbfb86 to ddaf20e Nov 22, 2016

@henri-tremblay

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2016

I should work on it today. I've pushed the rebase. Now, it's basically a matter of fixing CacheConfiguration I think.

@henri-tremblay henri-tremblay changed the title Hibernate 5.2, Ehcache 3, JCache support DO NOT MERGE Hibernate 5.2, Ehcache 3, JCache support Nov 23, 2016

@henri-tremblay

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2016

Everything seems to work now. Let's see if the build agrees

@jdubois
Copy link
Member

left a comment

This really looks awesome!!!!

  • The UI stuff will need to be ported to ng-2 (We'll do this later)
  • I'll try to merge this ASAP
  • This would make our next release

Thanks!!!!!!!

@jdubois jdubois merged commit 98dae51 into jhipster:master Nov 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jdubois

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

I changed very few things, and all my tests worked, so this is merged!!!

@cbornet

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

🎉
Now we need a clear migration tutorial for the liquibase checksum calculation change !

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