FINERACT-1724: Making global configuration caching transaction bound - #3041
Conversation
|
@galovics Kindly asking you to check the failing tests and squash the commits! |
e375a61 to
7f4e7e8
Compare
7f4e7e8 to
bc1e758
Compare
| @@ -29,6 +32,8 @@ | |||
|
|
|||
| public class ExtendedJpaTransactionManager extends JpaTransactionManager { | |||
There was a problem hiding this comment.
Do we really need this? Can't this be accomplished with https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/transaction/event/TransactionalEventListener.html (aka transaction bound events)? From a first look at this I'd say yes... if you want to keep those custom lifecycle callbacks then you just need to write a transaction event listener and do pretty much the same you do here in the extended transaction manager.
| this.repository = repository; | ||
| } | ||
|
|
||
| @Cacheable(value = "configByName", key = "T(org.apache.fineract.infrastructure.core.service.ThreadLocalContextUtil).getTenant().getTenantIdentifier().concat(#propertyName)") |
There was a problem hiding this comment.
Just in general: I would try to eliminate this service all together... as can be seen with this requirement to add caching... this thing is just getting in the way and it's not really dev-ops friendly (as you need a database in the first place to be able to do your configuration... instead of simple files)... just saying it again: I think reloadable configurations are a solved problem in Spring/Boot... my 2 cents here: this service is creating more head-aches than it provides solutions. But maybe a discussion for another day.
| @Slf4j | ||
| @Service | ||
| @RequiredArgsConstructor | ||
| public class ConfigurationDomainServiceJpa implements ConfigurationDomainService { |
There was a problem hiding this comment.
... see my comment on global configuration service... pretty much the same here.
| return Arrays.asList(noCache, singleNodeCache); | ||
| } | ||
|
|
||
| public Map<String, Object> switchToCache(final boolean ehcacheEnabled, final CacheType toCacheType) { |
There was a problem hiding this comment.
Probably I missed (a lot of) the conversation around this feature... but is there really a use case where you have to switch cache implementation during runtime? That seems to me such an important architectural decision that I would say it is done way before any instance of Fineract is running, at the least you would decide if EH Cache is enough or if you need it at all (I'd say the answer here is always yes... but not sure if you guys discussed a use case where no cache is desirable).
BTW: multi-node cache... Redis is your friend... 1st class support in Spring Boot and works really great (read: performant).
There was a problem hiding this comment.
For multi node cache for defo we should use Redis
4 participants
Description
Describe the changes made and why they were made.
Ignore if these details are present on the associated Apache Fineract JIRA ticket.
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.