ISPN-8024 Preview Hibernate cache local tutorial #29

galderz · 2017-07-07T16:01:52Z

Preview PR for the first of the Hibernate cache provider tutorials.

This first tutorial is focused on using Hibernate cache provider in a standalone, single-node, environment.

The tutorial randomly shows different counts, which I need to debug further. Also, the PR uses a snapshot, so do not yet integrate it.

Let's just use this PR to get some feedback on the tutorial itself.

galderz · 2017-07-07T16:03:24Z

I plan to add more simple tutorials:

a standalone cluster tutorial
a spring tutorial (probably just focus on the cluster functionality for spring)
ideally I'd like to add wildfly tutorial too...

galderz · 2017-07-09T02:59:39Z

One option I was considering is that before any operation is executed, reset the statistics, so that the changes in values are more visible and require less mental exercise for the reader. Thoughts?

galderz · 2017-07-09T10:50:28Z

Added a new commit that makes the tutorial expectations more predictable. See comments in commit log and code.

Also, on top of the stats reset before each operation, I'm also considering adding some assertions so that we can quickly verify it's all working fine (as opposed to having to read the log messages). I think the log messages are still useful for the reader.

I think we should have both message information and assertions so that even if assertions are disabled, the reader can verify manually the expected counts.

rvansa · 2017-07-17T15:52:59Z

...java/org/infinispan/tutorial/simple/hibernate/cache/local/InfinispanHibernateCacheLocal.java

+      EntityManagerFactory emf = Persistence.createEntityManagerFactory("events");
+
+      // Persist 3 entities, stats should show 3 second level cache puts
+      SecondLevelCacheStatistics cacheStats = persistEntities(emf);


Returning statistics from a write method looks ugly. Fetch stats independently.

rvansa · 2017-07-17T15:55:08Z

...java/org/infinispan/tutorial/simple/hibernate/cache/local/InfinispanHibernateCacheLocal.java

+   }
+
+   private static SecondLevelCacheStatistics getCacheStatistics(
+         String regionName, EntityManager em) {


I dislike passing EntityManager into a method that does not use the EntityManager. Pass EntityManagerFactory and call unwrap(SessionFactory.class) on that.

rvansa · 2017-07-17T16:00:10Z

...java/org/infinispan/tutorial/simple/hibernate/cache/local/InfinispanHibernateCacheLocal.java

+   private static SecondLevelCacheStatistics findEntity(EntityManagerFactory emf) {
+      EntityManager em = emf.createEntityManager();
+      try {
+         Event event = em.find(Event.class, 1L);


Relying on auto-generated ids does not seem stable (especially since you need to provide the L suffix), but probably good enough for tutorial. I would pass 1L as a parameter, though, to be symmetric with updateEntity.

rvansa · 2017-07-17T16:01:07Z

...java/org/infinispan/tutorial/simple/hibernate/cache/local/InfinispanHibernateCacheLocal.java

+         tx.begin();
+
+         Event event = em.find(Event.class, id);
+         String newName = "Caught a Snorlax!!";


rvansa · 2017-07-17T16:03:34Z

...java/org/infinispan/tutorial/simple/hibernate/cache/local/InfinispanHibernateCacheLocal.java

+   private static void evictEntity(EntityManagerFactory emf) {
+      EntityManager em = emf.createEntityManager();
+      try {
+         em.getEntityManagerFactory().getCache().evict(Event.class, 1L);


1L as param, same as findEntity

rvansa · 2017-07-17T16:08:53Z

...java/org/infinispan/tutorial/simple/hibernate/cache/local/InfinispanHibernateCacheLocal.java

+
+
+      // Save cache-expiring entity, stats should show a second level cache put
+      cacheStats = saveExpiringEntity(emf);


it's a bit surprising that cacheStats now belong to different entity cache.

* Added a short delay after deleting an entity so that there's a gap between the query invalidation and the query being cached. This makes the query afterwards not be considered out of date. * Added log4j2 dependency and configuration file for easy to generate easy to read TRACE logs for debugging. Java logging can do that but can't print thread names, so better to go with log4j2.

galderz · 2017-09-05T08:24:59Z

@rvansa I've fixed all of your concerns.

What do you think of my suggestion to reset statistics to make changes more clear?

I'm also considering adding Java assert calls so that if someone runs with -ea, the runtime can verify those assertions. I would still leave the text expect messages for reference.

rvansa · 2017-09-05T08:49:38Z

@galderz I don't think resetting the statistics is necessary, but I don't have any objections. If you want to add those assertions, refactor System.out.printf(...) + assertion to separate method. Though, this is not supposed to be a test.

One more thing about closing the em after commit - commit can throw an exception, and you should close the em anyway. Could you use try-with-resources to autoclose it?

galderz · 2017-09-15T10:54:52Z

Thx @rvansa!

I'll try to apply statistics reset and see if that makes clearer.

Yeah, I know it's not a test but using assert is free in this case, no need for dependency, so it offers an extra layer for little cost. Yeah, I'd do a refactor like the one you suggest.

Yup, I'll try to use the try-with-resources.

* It helps to see more clearly what are the effects of each JPA operation on the statistics, particularly when a JPA operation results in a count change of more than 1.

galderz · 2017-10-20T17:23:13Z

@rvansa I've pushed some new commits to add:

Assert calls to verify expected number.
If an assert failed, the entity manager factory was being left unclosed. To avoid a huge try/finally in the main method, I made the entity manager factory a static variable.
Clear stats when creating entity manager. I think this really clears it up the impact of each operation.
Finally, switched to try-with-resources for entity manager, but since EntityManager does not extend AutoClosable, I've just used Hibernate's Session.

If this looks good, let's integrate it and I can start work on next case, local second level cache with Spring or Wildfly.

rvansa · 2017-10-25T11:10:51Z

...java/org/infinispan/tutorial/simple/hibernate/cache/local/InfinispanHibernateCacheLocal.java

+      return emf.unwrap(SessionFactory.class).getStatistics();
+   }
+
+   private static void printfAssert(String format, long actual, int expected) {


actual is long, expected is int

rvansa · 2017-10-25T11:11:33Z

...java/org/infinispan/tutorial/simple/hibernate/cache/local/InfinispanHibernateCacheLocal.java

+   }
+
+   private static void findEntity(long id) {
+      try(Session em = createEntityManagerWithStatsCleared()) {


nitpicking, there should be a space between try and (

rvansa · 2017-10-25T11:13:37Z

...java/org/infinispan/tutorial/simple/hibernate/cache/local/InfinispanHibernateCacheLocal.java

+   private static Session createEntityManagerWithStatsCleared() {
+      EntityManager em = emf.createEntityManager();
+      emf.unwrap(SessionFactory.class).getStatistics().clear();
+      return em.unwrap(Session.class);


I think that you should add a comment that EntityMangager is not AutoCloseable to explain why are you using a Session instead... unwrap to get stats is to-be-expected, but using it for EM looks hackish.

rvansa · 2017-10-25T11:15:26Z

hibernate-cache-local/pom.xml

+               <arguments>
+                  <argument>-Djava.net.preferIPv4Stack=true</argument>
+                  <argument>-Djava.util.logging.config.file=src/main/resources/logging.properties</argument>
+                  <argument>-classpath</argument>


I think that you're missing -ea switch.

Didn't add it since assertions are no longer needed to verify counts.

rvansa · 2017-10-25T11:17:00Z

...java/org/infinispan/tutorial/simple/hibernate/cache/local/InfinispanHibernateCacheLocal.java

+
+   private static void printfAssert(String format, long actual, int expected) {
+      System.out.printf(format, actual, expected);
+      assert expected == actual : errorMsgAndCloseEntityManagerFactory(actual, expected);


Side effect (closing emf) from assert message looks clunky... use rather if ... throw some exception

Makes sense. Moving the code to simply throw exceptions and hence does not need assertions any more.

* Instead of having to rely on assertions, simply throw an exception when the expected effects do not happen. * Added javadoc to clarify Session unwrap call. * Minor formatting changes.

galderz · 2017-12-19T07:22:52Z

@rvansa I've pushed changes to handle your last comments.

galderz · 2018-01-08T12:29:46Z

I think we're good with this for now. I'm integrating and moving on to other examples.

galderz assigned tristantarrant Jul 7, 2017

galderz requested review from tristantarrant and rvansa July 7, 2017 16:01

galderz requested a review from Sanne July 7, 2017 16:15

galderz added the Ready for Review label Jul 9, 2017

rvansa suggested changes Jul 17, 2017

View reviewed changes

galderz added 7 commits September 5, 2017 09:03

ISPN-8024 Preview Hibernate cache local tutorial

0d2f48e

ISPN-8024 Fetch statistics independently

0d069bc

ISPN-8024 Pass in entity id being looked up

bd333b2

ISPN-8024 Remove unused variable

74111ef

ISPN-8024 Pass in entity ids as parameters

dff36b8

ISPN-8024 Extract SessionFactory directly from EntityManagerFactory

8f300f9

galderz force-pushed the preview_hibernate_cache_local branch from f506cc1 to 8f300f9 Compare September 5, 2017 08:23

rvansa approved these changes Sep 5, 2017

View reviewed changes

galderz added 4 commits October 20, 2017 18:09

ISPN-8024 Add assert calls to verify expected numbers

e97bfbc

ISPN-8024 Clear stats whenever an entity manager is created

597254d

* It helps to see more clearly what are the effects of each JPA operation on the statistics, particularly when a JPA operation results in a count change of more than 1.

ISPN-8024 Close EntityManagerFactory if assert error

8e20b41

ISPN-8024 Switched to Session to use try-with-resources

2fc5337

rvansa suggested changes Oct 25, 2017

View reviewed changes

ISPN-8024 Remove -ea requirement, clarify code and style

7a87f1e

* Instead of having to rely on assertions, simply throw an exception when the expected effects do not happen. * Added javadoc to clarify Session unwrap call. * Minor formatting changes.

galderz force-pushed the preview_hibernate_cache_local branch from 3969c9b to 7a87f1e Compare December 19, 2017 07:22

galderz merged commit 804cc39 into master Jan 8, 2018

galderz deleted the preview_hibernate_cache_local branch January 22, 2018 11:17



		// Save cache-expiring entity, stats should show a second level cache put
		cacheStats = saveExpiringEntity(emf);

ISPN-8024 Preview Hibernate cache local tutorial #29

ISPN-8024 Preview Hibernate cache local tutorial #29

Uh oh!

Conversation

galderz commented Jul 7, 2017

Uh oh!

galderz commented Jul 7, 2017

Uh oh!

galderz commented Jul 9, 2017

Uh oh!

galderz commented Jul 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galderz commented Sep 5, 2017

Uh oh!

rvansa commented Sep 5, 2017

Uh oh!

galderz commented Sep 15, 2017

Uh oh!

galderz commented Oct 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galderz commented Dec 19, 2017

Uh oh!

galderz commented Jan 8, 2018

Uh oh!

Uh oh!