You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by "budaidev (via GitHub)" <gi...@apache.org> on 2025/04/11 20:07:57 UTC

[PR] FINERACT-2234: extra caches for frequent db queries [fineract]

budaidev opened a new pull request, #4570:
URL: https://github.com/apache/fineract/pull/4570

   ## Description
   
   Adding extra caches to frequent database queries during the interest recalculation. 
   The 2 extra caches are:
   - calendar: this can be questionable
   - currencies: this one is definitely a win, since they don't change often
   
   Also added a simple performance test, to check out the differences locally.
   
   Local performance test results:
   ![compare_2](https://github.com/user-attachments/assets/7465fd4e-ee34-4c5a-be36-5c36463caec1)
   
   
   Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
   
   
   ## Checklist
   
   Please make sure these boxes are checked before submitting your pull request - thanks!
   
   - [ ] Write the commit message as per https://github.com/apache/fineract/#pull-requests
   
   - [ ] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
   
   - [ ] Create/update unit or integration tests for verifying the changes made.
   
   - [ ] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
   
   - [ ] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
   
   - [ ] Submission is not a "code dump".  (Large changes can be made "in repository" via a branch.  Ask on the developer mailing list for guidance, if required.)
   
   FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "budaidev (via GitHub)" <gi...@apache.org>.
budaidev commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2060619554


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -25,15 +25,23 @@
 import org.apache.fineract.portfolio.loanaccount.domain.Loan;
 import org.apache.fineract.portfolio.loanaccount.domain.LoanStatus;
 import org.apache.fineract.portfolio.savings.domain.SavingsAccount;
+import org.springframework.cache.annotation.CacheConfig;
+import org.springframework.cache.annotation.CacheEvict;
+import org.springframework.cache.annotation.Cacheable;
 import org.springframework.data.jpa.repository.JpaRepository;
 import org.springframework.data.jpa.repository.JpaSpecificationExecutor;
 import org.springframework.data.jpa.repository.Query;
 import org.springframework.data.repository.query.Param;
+import org.springframework.stereotype.Repository;
 
+@Repository
+@CacheConfig(cacheNames = "calendarInstances")
 public interface CalendarInstanceRepository extends JpaRepository<CalendarInstance, Long>, JpaSpecificationExecutor<CalendarInstance> {
 
+    @Cacheable(key = "T(java.util.Objects).hash(#calendarId, #entityId, #entityTypeId)")
     CalendarInstance findByCalendarIdAndEntityIdAndEntityTypeId(Long calendarId, Long entityId, Integer entityTypeId);

Review Comment:
   Done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #4570:
URL: https://github.com/apache/fineract/pull/4570


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2058244113


##########
fineract-core/src/main/java/org/apache/fineract/organisation/monetary/domain/ApplicationCurrencyRepository.java:
##########
@@ -28,16 +31,57 @@
 import org.springframework.stereotype.Repository;
 
 @Repository
+@CacheConfig(cacheNames = "currencies")
 public interface ApplicationCurrencyRepository
         extends JpaRepository<ApplicationCurrency, Long>, JpaSpecificationExecutor<ApplicationCurrency> {
 
     String FIND_CURRENCY_DETAILS = "SELECT new org.apache.fineract.organisation.monetary.data.CurrencyData(ac.code, ac.name, ac.decimalPlaces, ac.inMultiplesOf, ac.displaySymbol, ac.nameCode) FROM ApplicationCurrency ac ";
 
+    @Cacheable(key = "#currencyCode")
     ApplicationCurrency findOneByCode(String currencyCode);
 
+    @Cacheable(key = "#currencyCode")

Review Comment:
   I think it is not to use the very same cache key (currencyCode) of the very same cache (currencies). It is conflicting imho.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "oleksii-novikov-onix (via GitHub)" <gi...@apache.org>.
oleksii-novikov-onix commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2050987676


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -45,24 +53,56 @@ public interface CalendarInstanceRepository extends JpaRepository<CalendarInstan
      *            {@link CalendarType}
      * @return
      */
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId, #calendarTypeId)")
     CalendarInstance findByEntityIdAndEntityTypeIdAndCalendarTypeId(Long entityId, Integer entityTypeId, Integer calendarTypeId);
 
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId)")
     @Query("select ci from CalendarInstance ci where ci.entityId = :entityId and ci.entityTypeId = :entityTypeId")
     CalendarInstance findCalendarInstaneByEntityId(@Param("entityId") Long entityId, @Param("entityTypeId") Integer entityTypeId);
 
+    @Cacheable(key = "T(java.util.Objects).hash(#calendarId, #entityTypeId)")
     Collection<CalendarInstance> findByCalendarIdAndEntityTypeId(Long calendarId, Integer entityTypeId);
 
     /** Should use in clause, can I do it without creating a new class? **/
+    @Cacheable(key = "T(java.util.Objects).hash(#groupId, #clientId, #loanStatuses)")
     @Query("select ci from CalendarInstance ci where ci.entityId in (select loan.id from Loan loan where loan.client.id = :clientId and loan.group.id = :groupId and loan.loanStatus in :loanStatuses) and ci.entityTypeId = 3")
     List<CalendarInstance> findCalendarInstancesForLoansByGroupIdAndClientIdAndStatuses(@Param("groupId") Long groupId,
             @Param("clientId") Long clientId, @Param("loanStatuses") Collection<LoanStatus> loanStatuses);
 
     /**
      * EntityType = 3 is for loan
      */
-
+    @Cacheable(key = "T(java.util.Objects).hash(#calendarId, #loanStatuses)")
     @Query("SELECT COUNT(ci.id) FROM CalendarInstance ci, Loan loan WHERE loan.id = ci.entityId AND ci.entityTypeId = 3 AND ci.calendar.id = :calendarId AND loan.loanStatus IN :loanStatuses ")
     Integer countOfLoansSyncedWithCalendar(@Param("calendarId") Long calendarId,
             @Param("loanStatuses") Collection<LoanStatus> loanStatuses);
 
+    // Override JpaRepository methods to add cache eviction
+    @Override
+    @CacheEvict(allEntries = true)
+    <S extends CalendarInstance> S save(S entity);

Review Comment:
   `@CacheEvict` added for all major operations like save, delete, etc., but is missing cache eviction for the `saveAndFlush `method. Please check, we can easily use `saveAndFlush ` somewhere. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #4570:
URL: https://github.com/apache/fineract/pull/4570#issuecomment-2820854786

   @budaidev Please review the comments and rebase your PR!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2058244113


##########
fineract-core/src/main/java/org/apache/fineract/organisation/monetary/domain/ApplicationCurrencyRepository.java:
##########
@@ -28,16 +31,57 @@
 import org.springframework.stereotype.Repository;
 
 @Repository
+@CacheConfig(cacheNames = "currencies")
 public interface ApplicationCurrencyRepository
         extends JpaRepository<ApplicationCurrency, Long>, JpaSpecificationExecutor<ApplicationCurrency> {
 
     String FIND_CURRENCY_DETAILS = "SELECT new org.apache.fineract.organisation.monetary.data.CurrencyData(ac.code, ac.name, ac.decimalPlaces, ac.inMultiplesOf, ac.displaySymbol, ac.nameCode) FROM ApplicationCurrency ac ";
 
+    @Cacheable(key = "#currencyCode")
     ApplicationCurrency findOneByCode(String currencyCode);
 
+    @Cacheable(key = "#currencyCode")

Review Comment:
   I think it is not right to use the very same cache key (currencyCode) of the very same cache (currencies), but different response objects. It is conflicting imho.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "oleksii-novikov-onix (via GitHub)" <gi...@apache.org>.
oleksii-novikov-onix commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2051000794


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -45,24 +52,27 @@ public interface CalendarInstanceRepository extends JpaRepository<CalendarInstan
      *            {@link CalendarType}
      * @return
      */
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId, #calendarTypeId)")
     CalendarInstance findByEntityIdAndEntityTypeIdAndCalendarTypeId(Long entityId, Integer entityTypeId, Integer calendarTypeId);
 
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId)")
     @Query("select ci from CalendarInstance ci where ci.entityId = :entityId and ci.entityTypeId = :entityTypeId")
     CalendarInstance findCalendarInstaneByEntityId(@Param("entityId") Long entityId, @Param("entityTypeId") Integer entityTypeId);

Review Comment:
   findByEntityIdAndEntityTypeId and findCalendarInstanceByEntityId use the same key pattern, probably this could lead to a collision, right?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2058244113


##########
fineract-core/src/main/java/org/apache/fineract/organisation/monetary/domain/ApplicationCurrencyRepository.java:
##########
@@ -28,16 +31,57 @@
 import org.springframework.stereotype.Repository;
 
 @Repository
+@CacheConfig(cacheNames = "currencies")
 public interface ApplicationCurrencyRepository
         extends JpaRepository<ApplicationCurrency, Long>, JpaSpecificationExecutor<ApplicationCurrency> {
 
     String FIND_CURRENCY_DETAILS = "SELECT new org.apache.fineract.organisation.monetary.data.CurrencyData(ac.code, ac.name, ac.decimalPlaces, ac.inMultiplesOf, ac.displaySymbol, ac.nameCode) FROM ApplicationCurrency ac ";
 
+    @Cacheable(key = "#currencyCode")
     ApplicationCurrency findOneByCode(String currencyCode);
 
+    @Cacheable(key = "#currencyCode")

Review Comment:
   I think it is not right to use the very same cache key (currencyCode) of the very same cache (currencies). It is conflicting imho.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "budaidev (via GitHub)" <gi...@apache.org>.
budaidev commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2056004470


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -45,24 +52,27 @@ public interface CalendarInstanceRepository extends JpaRepository<CalendarInstan
      *            {@link CalendarType}
      * @return
      */
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId, #calendarTypeId)")
     CalendarInstance findByEntityIdAndEntityTypeIdAndCalendarTypeId(Long entityId, Integer entityTypeId, Integer calendarTypeId);
 
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId)")
     @Query("select ci from CalendarInstance ci where ci.entityId = :entityId and ci.entityTypeId = :entityTypeId")
     CalendarInstance findCalendarInstaneByEntityId(@Param("entityId") Long entityId, @Param("entityTypeId") Integer entityTypeId);

Review Comment:
   Added an extra seperation for collection and instance type to avoid collision



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -25,15 +25,22 @@
 import org.apache.fineract.portfolio.loanaccount.domain.Loan;
 import org.apache.fineract.portfolio.loanaccount.domain.LoanStatus;
 import org.apache.fineract.portfolio.savings.domain.SavingsAccount;
+import org.springframework.cache.annotation.CacheConfig;
+import org.springframework.cache.annotation.Cacheable;
 import org.springframework.data.jpa.repository.JpaRepository;
 import org.springframework.data.jpa.repository.JpaSpecificationExecutor;
 import org.springframework.data.jpa.repository.Query;
 import org.springframework.data.repository.query.Param;
+import org.springframework.stereotype.Repository;
 
+@Repository
+@CacheConfig(cacheNames = "calendarInstances")

Review Comment:
   added



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "budaidev (via GitHub)" <gi...@apache.org>.
budaidev commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2058340312


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -45,24 +52,27 @@ public interface CalendarInstanceRepository extends JpaRepository<CalendarInstan
      *            {@link CalendarType}
      * @return
      */
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId, #calendarTypeId)")
     CalendarInstance findByEntityIdAndEntityTypeIdAndCalendarTypeId(Long entityId, Integer entityTypeId, Integer calendarTypeId);

Review Comment:
   True, I am checking the best practices for this case



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2058265786


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -45,24 +52,27 @@ public interface CalendarInstanceRepository extends JpaRepository<CalendarInstan
      *            {@link CalendarType}
      * @return
      */
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId, #calendarTypeId)")
     CalendarInstance findByEntityIdAndEntityTypeIdAndCalendarTypeId(Long entityId, Integer entityTypeId, Integer calendarTypeId);

Review Comment:
   I think we should use different key generation logic as i think with the hash calculation we can easily conflict:
   
   `@Cacheable(key = "T(java.util.Objects).hash(#calendarId, #entityId, #entityTypeId)")
       CalendarInstance findByCalendarIdAndEntityIdAndEntityTypeId(Long calendarId, Long entityId, Integer entityTypeId);`
   vs
   `@Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId, #calendarTypeId)")
       CalendarInstance findByEntityIdAndEntityTypeIdAndCalendarTypeId(Long entityId, Integer entityTypeId, Integer calendarTypeId);`
       
   What if:
   - calendarId: 1
   - entityId: 1
   - entityTypeId: 1
   - calendarTypeId: 1
   
   Hash of 1,1,1 and Hash of 1,1,1....



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "budaidev (via GitHub)" <gi...@apache.org>.
budaidev commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2055906787


##########
fineract-core/src/main/java/org/apache/fineract/organisation/monetary/domain/ApplicationCurrencyRepository.java:
##########
@@ -28,16 +30,20 @@
 import org.springframework.stereotype.Repository;
 
 @Repository
+@CacheConfig(cacheNames = "currencies")
 public interface ApplicationCurrencyRepository
         extends JpaRepository<ApplicationCurrency, Long>, JpaSpecificationExecutor<ApplicationCurrency> {
 
     String FIND_CURRENCY_DETAILS = "SELECT new org.apache.fineract.organisation.monetary.data.CurrencyData(ac.code, ac.name, ac.decimalPlaces, ac.inMultiplesOf, ac.displaySymbol, ac.nameCode) FROM ApplicationCurrency ac ";
 
+    @Cacheable(key = "#currencyCode")
     ApplicationCurrency findOneByCode(String currencyCode);
 
+    @Cacheable(key = "#currencyCode")
     @Query(FIND_CURRENCY_DETAILS + " WHERE ac.code = :code")
     CurrencyData findCurrencyDataByCode(@Param("code") String currencyCode);
 
+    @Cacheable
     @Query(FIND_CURRENCY_DETAILS)
     List<CurrencyData> findAllSorted(Sort sort);

Review Comment:
   I added the CacheEvict for the other operations



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2058258809


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -25,15 +25,23 @@
 import org.apache.fineract.portfolio.loanaccount.domain.Loan;
 import org.apache.fineract.portfolio.loanaccount.domain.LoanStatus;
 import org.apache.fineract.portfolio.savings.domain.SavingsAccount;
+import org.springframework.cache.annotation.CacheConfig;
+import org.springframework.cache.annotation.CacheEvict;
+import org.springframework.cache.annotation.Cacheable;
 import org.springframework.data.jpa.repository.JpaRepository;
 import org.springframework.data.jpa.repository.JpaSpecificationExecutor;
 import org.springframework.data.jpa.repository.Query;
 import org.springframework.data.repository.query.Param;
+import org.springframework.stereotype.Repository;
 
+@Repository
+@CacheConfig(cacheNames = "calendarInstances")
 public interface CalendarInstanceRepository extends JpaRepository<CalendarInstance, Long>, JpaSpecificationExecutor<CalendarInstance> {
 
+    @Cacheable(key = "T(java.util.Objects).hash(#calendarId, #entityId, #entityTypeId)")
     CalendarInstance findByCalendarIdAndEntityIdAndEntityTypeId(Long calendarId, Long entityId, Integer entityTypeId);

Review Comment:
   I think using a string-composed key would be more practical here. I know hash collision is rare, but is it worth risking it?
   
   Something like this:
   `@Cacheable(key = "'ci_cId' + #calendarId + '_eId' + #entityId + '_eTId' + #entityTypeId")` ?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2058258809


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -25,15 +25,23 @@
 import org.apache.fineract.portfolio.loanaccount.domain.Loan;
 import org.apache.fineract.portfolio.loanaccount.domain.LoanStatus;
 import org.apache.fineract.portfolio.savings.domain.SavingsAccount;
+import org.springframework.cache.annotation.CacheConfig;
+import org.springframework.cache.annotation.CacheEvict;
+import org.springframework.cache.annotation.Cacheable;
 import org.springframework.data.jpa.repository.JpaRepository;
 import org.springframework.data.jpa.repository.JpaSpecificationExecutor;
 import org.springframework.data.jpa.repository.Query;
 import org.springframework.data.repository.query.Param;
+import org.springframework.stereotype.Repository;
 
+@Repository
+@CacheConfig(cacheNames = "calendarInstances")
 public interface CalendarInstanceRepository extends JpaRepository<CalendarInstance, Long>, JpaSpecificationExecutor<CalendarInstance> {
 
+    @Cacheable(key = "T(java.util.Objects).hash(#calendarId, #entityId, #entityTypeId)")
     CalendarInstance findByCalendarIdAndEntityIdAndEntityTypeId(Long calendarId, Long entityId, Integer entityTypeId);

Review Comment:
   I think using a string-composed key would be more practical here. I know hash collision is rare, but is it worth risking it?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "galovics (via GitHub)" <gi...@apache.org>.
galovics commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2040593315


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -25,15 +25,22 @@
 import org.apache.fineract.portfolio.loanaccount.domain.Loan;
 import org.apache.fineract.portfolio.loanaccount.domain.LoanStatus;
 import org.apache.fineract.portfolio.savings.domain.SavingsAccount;
+import org.springframework.cache.annotation.CacheConfig;
+import org.springframework.cache.annotation.Cacheable;
 import org.springframework.data.jpa.repository.JpaRepository;
 import org.springframework.data.jpa.repository.JpaSpecificationExecutor;
 import org.springframework.data.jpa.repository.Query;
 import org.springframework.data.repository.query.Param;
+import org.springframework.stereotype.Repository;
 
+@Repository
+@CacheConfig(cacheNames = "calendarInstances")

Review Comment:
   Cache eviction here as well? 



##########
fineract-core/src/main/java/org/apache/fineract/organisation/monetary/domain/ApplicationCurrencyRepository.java:
##########
@@ -28,16 +30,20 @@
 import org.springframework.stereotype.Repository;
 
 @Repository
+@CacheConfig(cacheNames = "currencies")
 public interface ApplicationCurrencyRepository
         extends JpaRepository<ApplicationCurrency, Long>, JpaSpecificationExecutor<ApplicationCurrency> {
 
     String FIND_CURRENCY_DETAILS = "SELECT new org.apache.fineract.organisation.monetary.data.CurrencyData(ac.code, ac.name, ac.decimalPlaces, ac.inMultiplesOf, ac.displaySymbol, ac.nameCode) FROM ApplicationCurrency ac ";
 
+    @Cacheable(key = "#currencyCode")
     ApplicationCurrency findOneByCode(String currencyCode);
 
+    @Cacheable(key = "#currencyCode")
     @Query(FIND_CURRENCY_DETAILS + " WHERE ac.code = :code")
     CurrencyData findCurrencyDataByCode(@Param("code") String currencyCode);
 
+    @Cacheable
     @Query(FIND_CURRENCY_DETAILS)
     List<CurrencyData> findAllSorted(Sort sort);

Review Comment:
   If I remember correctly there's a way to add new currencies to the system through the UI, so I think the cache eviction is missing from here.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -45,24 +52,27 @@ public interface CalendarInstanceRepository extends JpaRepository<CalendarInstan
      *            {@link CalendarType}
      * @return
      */
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId, #calendarTypeId)")
     CalendarInstance findByEntityIdAndEntityTypeIdAndCalendarTypeId(Long entityId, Integer entityTypeId, Integer calendarTypeId);
 
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId)")
     @Query("select ci from CalendarInstance ci where ci.entityId = :entityId and ci.entityTypeId = :entityTypeId")
     CalendarInstance findCalendarInstaneByEntityId(@Param("entityId") Long entityId, @Param("entityTypeId") Integer entityTypeId);

Review Comment:
   Isn't this also the same as the second method?



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -45,24 +52,27 @@ public interface CalendarInstanceRepository extends JpaRepository<CalendarInstan
      *            {@link CalendarType}
      * @return
      */
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId, #calendarTypeId)")
     CalendarInstance findByEntityIdAndEntityTypeIdAndCalendarTypeId(Long entityId, Integer entityTypeId, Integer calendarTypeId);

Review Comment:
   Btw isn't this the same as the first method?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "oleksii-novikov-onix (via GitHub)" <gi...@apache.org>.
oleksii-novikov-onix commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2051000794


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -45,24 +52,27 @@ public interface CalendarInstanceRepository extends JpaRepository<CalendarInstan
      *            {@link CalendarType}
      * @return
      */
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId, #calendarTypeId)")
     CalendarInstance findByEntityIdAndEntityTypeIdAndCalendarTypeId(Long entityId, Integer entityTypeId, Integer calendarTypeId);
 
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId)")
     @Query("select ci from CalendarInstance ci where ci.entityId = :entityId and ci.entityTypeId = :entityTypeId")
     CalendarInstance findCalendarInstaneByEntityId(@Param("entityId") Long entityId, @Param("entityTypeId") Integer entityTypeId);

Review Comment:
   `findByEntityIdAndEntityTypeId` and `findCalendarInstanceByEntityId` use the same key pattern, probably this could lead to a collision, right?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2058258809


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -25,15 +25,23 @@
 import org.apache.fineract.portfolio.loanaccount.domain.Loan;
 import org.apache.fineract.portfolio.loanaccount.domain.LoanStatus;
 import org.apache.fineract.portfolio.savings.domain.SavingsAccount;
+import org.springframework.cache.annotation.CacheConfig;
+import org.springframework.cache.annotation.CacheEvict;
+import org.springframework.cache.annotation.Cacheable;
 import org.springframework.data.jpa.repository.JpaRepository;
 import org.springframework.data.jpa.repository.JpaSpecificationExecutor;
 import org.springframework.data.jpa.repository.Query;
 import org.springframework.data.repository.query.Param;
+import org.springframework.stereotype.Repository;
 
+@Repository
+@CacheConfig(cacheNames = "calendarInstances")
 public interface CalendarInstanceRepository extends JpaRepository<CalendarInstance, Long>, JpaSpecificationExecutor<CalendarInstance> {
 
+    @Cacheable(key = "T(java.util.Objects).hash(#calendarId, #entityId, #entityTypeId)")
     CalendarInstance findByCalendarIdAndEntityIdAndEntityTypeId(Long calendarId, Long entityId, Integer entityTypeId);

Review Comment:
   I think the default key generation or using a string-composed key would be more practical here. I know hash collision is rare, but is it worth risking it?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "budaidev (via GitHub)" <gi...@apache.org>.
budaidev commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2040743574


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -45,24 +52,27 @@ public interface CalendarInstanceRepository extends JpaRepository<CalendarInstan
      *            {@link CalendarType}
      * @return
      */
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId, #calendarTypeId)")
     CalendarInstance findByEntityIdAndEntityTypeIdAndCalendarTypeId(Long entityId, Integer entityTypeId, Integer calendarTypeId);

Review Comment:
   > Btw isn't this the same as the first method?
   
   In the calendarInstance entity there are 3 columns: calendar_id, entity_id, entity_type_enum
   The calendar entity which is connected with the calendarInstance have many fields, including the typeId.
   Now in the test I see that the entity_type_enum is not the same as calendar_type_enum, so they might have probably different purpose
   



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -45,24 +52,27 @@ public interface CalendarInstanceRepository extends JpaRepository<CalendarInstan
      *            {@link CalendarType}
      * @return
      */
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId, #calendarTypeId)")
     CalendarInstance findByEntityIdAndEntityTypeIdAndCalendarTypeId(Long entityId, Integer entityTypeId, Integer calendarTypeId);
 
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId)")
     @Query("select ci from CalendarInstance ci where ci.entityId = :entityId and ci.entityTypeId = :entityTypeId")
     CalendarInstance findCalendarInstaneByEntityId(@Param("entityId") Long entityId, @Param("entityTypeId") Integer entityTypeId);

Review Comment:
   > Isn't this also the same as the second method?
   
   Same for here, calanderType <> entityType



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "budaidev (via GitHub)" <gi...@apache.org>.
budaidev commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2056005675


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -45,24 +53,56 @@ public interface CalendarInstanceRepository extends JpaRepository<CalendarInstan
      *            {@link CalendarType}
      * @return
      */
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId, #calendarTypeId)")
     CalendarInstance findByEntityIdAndEntityTypeIdAndCalendarTypeId(Long entityId, Integer entityTypeId, Integer calendarTypeId);
 
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId)")
     @Query("select ci from CalendarInstance ci where ci.entityId = :entityId and ci.entityTypeId = :entityTypeId")
     CalendarInstance findCalendarInstaneByEntityId(@Param("entityId") Long entityId, @Param("entityTypeId") Integer entityTypeId);
 
+    @Cacheable(key = "T(java.util.Objects).hash(#calendarId, #entityTypeId)")
     Collection<CalendarInstance> findByCalendarIdAndEntityTypeId(Long calendarId, Integer entityTypeId);
 
     /** Should use in clause, can I do it without creating a new class? **/
+    @Cacheable(key = "T(java.util.Objects).hash(#groupId, #clientId, #loanStatuses)")
     @Query("select ci from CalendarInstance ci where ci.entityId in (select loan.id from Loan loan where loan.client.id = :clientId and loan.group.id = :groupId and loan.loanStatus in :loanStatuses) and ci.entityTypeId = 3")
     List<CalendarInstance> findCalendarInstancesForLoansByGroupIdAndClientIdAndStatuses(@Param("groupId") Long groupId,
             @Param("clientId") Long clientId, @Param("loanStatuses") Collection<LoanStatus> loanStatuses);
 
     /**
      * EntityType = 3 is for loan
      */
-
+    @Cacheable(key = "T(java.util.Objects).hash(#calendarId, #loanStatuses)")
     @Query("SELECT COUNT(ci.id) FROM CalendarInstance ci, Loan loan WHERE loan.id = ci.entityId AND ci.entityTypeId = 3 AND ci.calendar.id = :calendarId AND loan.loanStatus IN :loanStatuses ")
     Integer countOfLoansSyncedWithCalendar(@Param("calendarId") Long calendarId,
             @Param("loanStatuses") Collection<LoanStatus> loanStatuses);
 
+    // Override JpaRepository methods to add cache eviction
+    @Override
+    @CacheEvict(allEntries = true)
+    <S extends CalendarInstance> S save(S entity);

Review Comment:
   I added a saveAndFlush method with @CacheEvict annotation



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #4570:
URL: https://github.com/apache/fineract/pull/4570#discussion_r2058265786


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/calendar/domain/CalendarInstanceRepository.java:
##########
@@ -45,24 +52,27 @@ public interface CalendarInstanceRepository extends JpaRepository<CalendarInstan
      *            {@link CalendarType}
      * @return
      */
+    @Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId, #calendarTypeId)")
     CalendarInstance findByEntityIdAndEntityTypeIdAndCalendarTypeId(Long entityId, Integer entityTypeId, Integer calendarTypeId);

Review Comment:
   I think we should use different keygeneration logic as i think with hash calculation we can easily conflict:
   
   `@Cacheable(key = "T(java.util.Objects).hash(#calendarId, #entityId, #entityTypeId)")
       CalendarInstance findByCalendarIdAndEntityIdAndEntityTypeId(Long calendarId, Long entityId, Integer entityTypeId);`
   vs
   `@Cacheable(key = "T(java.util.Objects).hash(#entityId, #entityTypeId, #calendarTypeId)")
       CalendarInstance findByEntityIdAndEntityTypeIdAndCalendarTypeId(Long entityId, Integer entityTypeId, Integer calendarTypeId);`
       
   What if:
   - calendarId: 1
   - entityId: 1
   - entityTypeId: 1
   - calendarTypeId: 1
   
   Hash of 1,1,1 and Hash of 1,1,1....



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "galovics (via GitHub)" <gi...@apache.org>.
galovics commented on PR #4570:
URL: https://github.com/apache/fineract/pull/4570#issuecomment-2798695739

   > @budaidev just for the future if somebody looks back at the PR, do you mind explaning how you executed the load test and what tools you used? Thanks.
   
   Nevermind, I just reviewed the code and I saw you have the test ready to be run.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2234: extra caches for frequent db queries [fineract]

Posted by "galovics (via GitHub)" <gi...@apache.org>.
galovics commented on PR #4570:
URL: https://github.com/apache/fineract/pull/4570#issuecomment-2798693679

   @budaidev just for the future if somebody looks back at the PR, do you mind explaning how you executed the load test and what tools you used? Thanks.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org