Conversation
656a754
to
b4a3b87
Compare
<property name="hibernate.cache.use_second_level_cache" value="true"/> | ||
<property name="hibernate.cache.use_query_cache" value="true"/> | ||
<property name="hibernate.cache.region.factory_class" value="org.hibernate.cache.infinispan.JndiInfinispanRegionFactory" /> | ||
<property name="hibernate.cache.infinispan.cachemanager" value="java:CacheManager/entity"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work in "standalone" profile too ?
The GSOC branch (kafka) is able to run on that profile, and it would be great to keep that spirit :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should, but I will check to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is only relevant for the GSoC branch ;-) since master/1.1.x-dev do require the -full
profile, due to JMS
f627da9
to
d30c2fc
Compare
@@ -43,20 +43,20 @@ public void setEntityManager(EntityManager entityManager) { | |||
|
|||
|
|||
protected TypedQuery<T> createQuery(String jpql) { | |||
return entityManager.createQuery(jpql, getType()); | |||
return entityManager.createQuery(jpql, getType()).setHint("org.hibernate.cacheable", true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to cache all the queries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gunnarmorling mind taking a look ?
I think, in theory caching queries does seem reasonable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be cautious about enabling query caching in such common base class and query method. You are likely better off to enable this for dedicated entity types and queries only (which are unlikely to change often).
Also have you enabled the second-level cache before (query caching will only cache entity ids)? Check out the section on query caching here, it has some good info.
Overall, I think you first should analyse which queries are run very often and then make use of query caching more selectivel.y
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gunnarmorling Yes, second level cache has been enabled.
Since we don't have queries that return big amount of data (if we would have, if would be a bug), I was thinking that:
- If the query is seldom used, it will be just a few bytes and will be evicted as soon as the cache expires. Since it is just a few bytes, it shouldn't be an issue
- If the query is used often, than it is good for that to be cached
On the other side, if I choose myself which query to cache, I could miss some scenarios.
Again, since the data returned by the queries is pretty small, do the benefit of caching only some query worth the risk and the effort needed to choose them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to test, what's actually executed often
I think the following are the most important cases:
- lot's of (different) device metadata updates on the registry endpoint
- sending push messages (and the implicit calls to the Flat Metrics)
Than we have "management" API calls, from the UI (E.g. that loads the Application(s), with their Variants, with their Device installations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matzew : ok, I'm still loading the database to have some GB of data using the ups test suite form @josemigallas. While doing that, I'm checking what queries gets executed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating 1000 APPS, 1 VARIANT and 1 TOKEN
- 500 different SELECTS on ANDROID_VARIANT are executed. Each of them is executed 2 times with the same parameters.
- 1000 unique queries on INSTALLATION
- 3.998 unique queries on VARIANT
- 1.998 unique queries on PUSH_APPLICATION
For a TOTAL of 7996 query.
Creating 1 APP, 1000 VARIANTS and 0 TOKENS
- 3.000 selects on VARIANT are executed. 1 query is executed 2.000 times always with the same parameters
- 1.945 queries on PUSH_APPLICATION. 1 is executed 1000 times, another is executed 945 times, both with exactly the same parameters
Creating 1 APP, 1 VARIANT and 1000 TOKENS
- 1000 queries on INSTALLATION
- 1000 queries on ANDROID_VARIANT. It is the same query, executed 1000 times with the same parameters
- 2 queries on PUSH_APPLICATION. The same query is executed 2 times.
- 1002 queries on VARIANT. Here the same exact query is executed 1000 times joining all the variant types.
I currently have the exact query executed on MySQL. Going to match that with the JPA
code used by UPS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matzew @gunnarmorling Will update with the matched JPA query and with the data from the other scenario. A more structured document will be attached to this later.
@matzew would you please review? |
+1
…On Tue, Sep 19, 2017 at 10:46 AM, Massimiliano Ziccardi < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In model/jpa/src/main/java/org/jboss/aerogear/unifiedpush/
jpa/dao/impl/JPABaseDao.java
<#928 (comment)>
:
> @@ -43,20 +43,20 @@ public void setEntityManager(EntityManager entityManager) {
protected TypedQuery<T> createQuery(String jpql) {
- return entityManager.createQuery(jpql, getType());
+ return entityManager.createQuery(jpql, getType()).setHint("org.hibernate.cacheable", true);
@matzew <https://github.com/matzew> : ok, I'm still loading the database
to have some GB of data using the ups test suite form @josemigallas
<https://github.com/josemigallas>. While doing that, I'm checking what
queries gets executed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#928 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJnzpH4f7prjRswA1tD5_L3qJSjDBfhks5sj39ZgaJpZM4PQCH5>
.
--
Matthias Wessendorf
blog: http://matthiaswessendorf.wordpress.com/
twitter: http://twitter.com/mwessendorf
|
👍
…On Fri, Oct 13, 2017 at 4:02 PM, Massimiliano Ziccardi < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In model/jpa/src/main/java/org/jboss/aerogear/unifiedpush/
jpa/dao/impl/JPABaseDao.java
<#928 (comment)>
:
> @@ -43,20 +43,20 @@ public void setEntityManager(EntityManager entityManager) {
protected TypedQuery<T> createQuery(String jpql) {
- return entityManager.createQuery(jpql, getType());
+ return entityManager.createQuery(jpql, getType()).setHint("org.hibernate.cacheable", true);
@matzew <https://github.com/matzew> @gunnarmorling
<https://github.com/gunnarmorling> Will update with the matched JPA query
and with the data from the other scenario.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#928 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJnzsnpgNwDrnEdJato-LtT5gIjTpx4ks5sr21kgaJpZM4PQCH5>
.
--
Matthias Wessendorf
blog: http://matthiaswessendorf.wordpress.com/
twitter: http://twitter.com/mwessendorf
|
@ziccardi what's the status here ? should we close it ? |
@matzew changed the code to cache only relevant queries. Would you mind to review? |
<property name="hibernate.cache.use_query_cache" value="true"/> | ||
<property name="hibernate.cache.use_second_level_cache" value="true"/> | ||
|
||
<property name="hibernate.cache.infinispan.statistics" value="true"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this work out of the box ?
do we really want to update the 1.1.x branch ?
I'd perhap be happy to only work on master - unless we have to :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ziccardi any comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matzew : yes it works. about the 1.1.x branch, we were doing that on 1.1.x branch when we started this, but I can easily move it to master
@gunnarmorling mind taking a look too 👀 |
Hi, I'd be careful with query result caching, e.g. see this article for some reasons. If you haven't done so yet, I'd recommend to enable profiling to identify the most expensive queries and spend some time on optimizing them. |
Motivation
UPS database can grow up to become quite big.
Since many queries are executed, that could slow down the database and, thus, slow the whole platform.
Solution
We want to enable Hibernate cache so that queries are not executed again on the database if they have been already executed and still cached.