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.

Sign up for GitHub

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

Jump to bottom

Add hibernate caching, bump some dependency versions #289

Merged
merged 6 commits into from
Jan 23, 2018
Merged

Add hibernate caching, bump some dependency versions #289

merged 6 commits into from
Jan 23, 2018

Conversation

hwbllmnn
Copy link
Member

This updates some dependencies to recent versions, most notably log4j2 (log4j 1 is at EOL since ~2015).

Most notably, this adds hibernate cache annotations and also uses JOINs where possible. Also, GenericHibernateDao#createDistinctRootEntityCriteria uses setCacheable with the value of the hibernate.cache.use_query_cache property, effectively caching everything if set to true.

@terrestris/devs @buehner please review carefully

We probably should make a 0.1.4 release before merging this, as projects probably need some updates when using this version.

Alternatively we could create a 0.2.x release branch and merge this there.

@marcjansen
Copy link
Member

I like this, very nice work, thanks.

+1 for a release before merging this.

Can you update the archetype so we can see which changes would be needed there?

Do people need to change the log4j configurations they have?

Related: Is there any chance that we can control the @Fetch configuration at build time of the actual application, not at build time of this framework? I find it odd that we decide which is the best way in the framework, and I faintly remeber switching between configurations often. This last comment should not block this from being merged.

I think this deserves more review work from others.

@buehner
Copy link
Member

buehner commented Dec 19, 2017

I just had a very quick look at this for now and this PR looks really great! I'll have a deeper look at this later.

@hwbllmnn
Copy link
Member Author

Regarding the @Fetch configuration, you can override this manually when querying (eg. criteria.setFetchMode(path, FetchMode.SUBSELECT);).

The log4j configurations need to be adapted, unfortunately, but I provided a template in src/test/resources/log4j2.xml which is pretty straightforward IMHO.

Regarding the archetype, I'll have a look into it and report back.

@hwbllmnn
Copy link
Member Author

Archetype changes have been added in e2c27d9 (example log4j2.xml, hibernate.properties, default cache config).

weskamm
weskamm reviewed Dec 20, 2017
View reviewed changes
src/shogun2-core/src/test/resources/META-INF/hibernate.properties
@@ -5,7 +5,7 @@ hibernate.id.new_generator_mappings=true
hibernate.hbm2ddl.auto=create

# caching
#hibernate.cache.use_query_cache=true
hibernate.cache.use_query_cache=false
Copy link
Member

@weskamm weskamm Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

did you have issues having set this to true? if so, which ones?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, but since this was implicitly the default before, I thought I'd leave it at that. I just need the property to be set, since I'm referencing it in the code.

@weskamm
Copy link
Member

weskamm commented Dec 20, 2017

i have left just a minor question, looking good to me.

ahennr
ahennr approved these changes Dec 20, 2017
View reviewed changes
Copy link
Member

@ahennr ahennr left a comment

Choose a reason for hiding this comment

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

Very nice enhancement @hwbllmnn 🔝, LGTM!

I agree with @buehner and @marcjansen to release a new version before merging.

src/pom.xml Outdated
@@ -135,7 +135,7 @@
<spring.version>4.3.11.RELEASE</spring.version>
<spring-security.version>4.2.3.RELEASE</spring-security.version>
<!-- TODO: Raise log4j to version 2 -->
Copy link
Member

Choose a reason for hiding this comment

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

Very, very minor: This TODO is done by your change, isn't it? So it can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, removed it.

ahennr
ahennr reviewed Dec 20, 2017
View reviewed changes
src/pom.xml Outdated
@@ -135,7 +135,7 @@
<spring.version>4.3.11.RELEASE</spring.version>
<spring-security.version>4.2.3.RELEASE</spring-security.version>
<!-- TODO: Raise log4j to version 2 -->
<log4j.version>1.2.17</log4j.version>
<log4j.version>2.10.0</log4j.version>
Copy link
Member

@ahennr ahennr Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Did you add a log4j config file for the tests as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, for the tests I just stuck with the slf4j-simple logger. But I converted the log4j.properties example in the archetype to a new log4j2.xml.

@weskamm
Copy link
Member

weskamm commented Dec 20, 2017

I see a lot of deprecation warnings using this code in another project:

WARN [org.hibernate.orm.deprecation] - HHH90000022: Hibernate's legacy org.hibernate.Criteria API is deprecated; use the JPA javax.persistence.criteria.CriteriaQuery instead

I think this is related, could you please check this?

@hwbllmnn
Copy link
Member Author

@weskamm The warnings you're seeing are related to the projects' switch to Hibernate 5, where said API has become deprecated. I agree we should update SHOGun to use the JPA criteria API, but that's unrelated to this PR and also a lot of work 😿

@hwbllmnn
Copy link
Member Author

I've just added a commit with a few changes to the archetype so the generated webapp will start again with jetty. Changes include:

marcjansen
marcjansen approved these changes Dec 20, 2017
View reviewed changes
Copy link
Member

@marcjansen marcjansen left a comment

Choose a reason for hiding this comment

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

Nice work everyone!

@buehner
Copy link
Member

buehner commented Jan 22, 2018

@terrestris/devs

the release v0.1.4 is done, so this could be merged now. But i think we still need to remove openjdk7 from the .travis.yml as we won't support Java 7 anymore after merging this. Correct?

@ahennr
Copy link
Member

ahennr commented Jan 22, 2018

Absolutely, Java7 dependency needs to be removed.
Currently we only use OracleJDK8 in our travis builds. We should include OpenJDK8 as well, shouldn't we?

@ahennr ahennr mentioned this pull request Jan 22, 2018
Closed
buehner
buehner approved these changes Jan 23, 2018
View reviewed changes
src/shogun2-core/src/test/resources/META-INF/hibernate.properties
@@ -5,7 +5,7 @@ hibernate.id.new_generator_mappings=true
hibernate.hbm2ddl.auto=create

# caching
#hibernate.cache.use_query_cache=true
hibernate.cache.use_query_cache=false
Copy link
Member

Choose a reason for hiding this comment

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

Shouldnt this be true by default?

Copy link
Member

Choose a reason for hiding this comment

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

See #289 (review)

@buehner buehner merged commit 710e47a into terrestris:master Jan 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahennr ahennr ahennr approved these changes

@weskamm weskamm weskamm left review comments

@buehner buehner buehner approved these changes

@marcjansen marcjansen marcjansen approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hwbllmnn @marcjansen @buehner @weskamm @ahennr