You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "alberto-art3ch (via GitHub)" <gi...@apache.org> on 2026/09/23 01:58:43 UTC
[PR] FINERACT-2684: Transaction-bound cache clearing and load global configs once per transaction [fineract]
alberto-art3ch opened a new pull request, #6493:
URL: https://github.com/apache/fineract/pull/6493
## Description
Global configuration lookups (`c_configuration` by name) run on almost every request: ~17 single-row queries per
write request under load, the most executed statement of the platform. The `@Cacheable("configByName")` never
hits because the default `TransactionBoundCacheManager` clears a shared Ehcache instance on every transaction of
every thread, and it keeps doing so even after switching to SINGLE_NODE via `PUT /caches`.
Changes, keeping the guarantee that every transaction starts from the database:
- Transaction-bound cache entries are now stored per thread (`ThreadLocalCacheStore`) and cleared only by that
thread's transaction boundaries and at request start; no cross-thread clearing or locking.
- `RuntimeDelegatingCacheManager` forwards transaction hooks only to the active manager, so SINGLE_NODE is no
longer wiped by the transaction-bound one.
- `GlobalConfigurationRepositoryWrapper` loads the whole table (76 rows) once per transaction with a read-only
query and evicts it on writes; the single mutating caller uses a dedicated for-update lookup.
Measured (k6, PostgreSQL 18, 36k loans): configuration statements per write request 16.7 -> 5.8 (one 76-row round
trip per transaction instead of one per property), write p95 27 -> 28 ms, read mix p95 86-105 -> 90 ms. No
client-visible behaviour change. New unit tests cover hook forwarding and once-per-transaction loading.
[FINERACT-2684](https://issues.apache.org/jira/browse/FINERACT-2684)
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Write the commit message as per [our guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#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](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made.
- [ ] Follow our [coding conventions](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
- [ ] [This PR must not be a "code dump"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute).
- [ ] If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
- [ ] I followed the [AI Policy](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#ai-policy).
Your assigned reviewer(s) will follow our [guidelines for code reviews](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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2968: Transaction-bound cache clearing and load global configs once per transaction [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6493:
URL: https://github.com/apache/fineract/pull/6493#issuecomment-6502566048
@alberto-art3ch Please fix the failing build
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2968: Transaction-bound cache clearing and load global configs once per transaction [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #6493:
URL: https://github.com/apache/fineract/pull/6493#discussion_r4383750331
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/DatatableWriteServiceImpl.java:
##########
@@ -490,6 +488,11 @@ public void updateDatatable(final String datatableName, final JsonCommand comman
throw new PlatformServiceUnavailableException("error.msg.datatable.column.update.not.allowed",
"One of the data table columns contains null values", e);
}
+ // Every other failure must surface. Swallowing it reported success to the client while, on
+ // PostgreSQL, the transaction was already aborted: the next statement of the command pipeline
+ // then failed with an exception that depended on what ran first (a wrapped query error mapped to
+ // 403, a raw EclipseLink error from the audit merge to 500).
+ throw e;
Review Comment:
The PR description lists the datatable DDL error mapping as out of scope (separate PR), and the reply to the earlier review says it was removed. It is still here: the catch is now `RuntimeException` and ends with `throw e;`, where develop swallowed the exception.
This is client-visible. A `PUT /datatables/{name}` whose ALTER/index step fails for any other reason used to return success and now returns an error. That contradicts "No client-visible behaviour change", and there's no test for it. Please drop it from this PR and send it separately, as described.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/configuration/service/GlobalConfigurationWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -84,6 +84,8 @@ public void addSurveyConfig(final String name) {
try {
final GlobalConfigurationProperty ppi = GlobalConfigurationProperty.newSurveyConfiguration(name);
this.repository.save(ppi);
+ // The cached snapshot of all properties must not outlive a newly created one.
+ this.repository.removeFromCache();
Review Comment:
This eviction is right, but there's a second path that creates a configuration row and skips it: `DatatableWriteServiceImpl#registerDatatable` inserts into `c_configuration` with plain JDBC when the category is PPI, and never calls `removeFromCache()`.
In SINGLE_NODE mode, once any config read has cached `<tenant>::all`, registering a PPI datatable leaves `GET /configurations/name/{dt}` and `PUT /configurations/name/{dt}` returning 404 until the cache entry expires. On develop the per-name cache never cached a miss, so this didn't happen. Please evict there too (or route it through `addSurveyConfig`), ideally with a test.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2968: Transaction-bound cache clearing and load global configs once per transaction [fineract]
Posted by "alberto-art3ch (via GitHub) via issues" <is...@fineract.apache.org>.
alberto-art3ch commented on PR #6493:
URL: https://github.com/apache/fineract/pull/6493#issuecomment-6560761921
> Thanks @alberto-art3ch, everything from my previous round is addressed: the datatable catch and jersey-micrometer are out, read callers get `GlobalConfigurationPropertyData` copies, `propertyName` is gone, the two-thread / `reset()` / SINGLE_NODE tests are there, and the `CacheConfig` registration catch was a good one. The PPI register/deregister eviction from Adam's round looks right too.
>
> The only thing blocking is the red E2E shard 14, and I don't think it's flaky. It's caused by this PR.
>
> `WorkingCapitalDatatables.feature` C76736 / C76737 (rename `note` -> `wc_loan_id` / `wc_product_loan_id`, expected 403) now get a 500 from `PUT /datatables/{name}`. The same shard with the same feature passes on develop (runs 40974277816 and 41048781611). On develop `updateDatatable` swallows the failed ALTER, so on PG the transaction is already aborted and the client only got the 403 because the next statement to hit the DB was a global config lookup. With this PR that lookup is served from the transaction-scoped snapshot, so the next statement is something else and it comes back as a raw 500. That's exactly what the comment in your first iteration's datatable change described.
>
> So the "no client-visible behaviour change" claim doesn't hold for this path. I still don't want the datatable error handling hidden in this PR, but it needs to land first: send the `updateDatatable` fix as its own PR with a test, get it merged, then rebase this one so shard 14 goes green. If you'd rather keep them together after all, that's ok too, but then put it in as a separate commit, update the description, and add a test for the rename-to-reserved-name case. Thoughts?
>
> Minor: the title says [FINERACT-2968](https://issues.apache.org/jira/browse/FINERACT-2968) but the description links [FINERACT-2684](https://issues.apache.org/jira/browse/FINERACT-2684). Which one is it?
Thanks @galovics! I've updated the ticket in the description. It was raised before 1.16.0 so It was documented with the umbrella Jira ticket for that, now we are in 1.17.0
BTW. I've raised the #6801 for solving the pending issue
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2684: Transaction-bound cache clearing and load global configs once per transaction [fineract]
Posted by "alberto-art3ch (via GitHub)" <gi...@apache.org>.
alberto-art3ch commented on PR #6493:
URL: https://github.com/apache/fineract/pull/6493#issuecomment-6278510647
> Hi @alberto-art3ch, thanks for the PR. The per-thread store and forwarding the hooks only to the active manager are a nice fix. I'd like these addressed first:
>
> * `DatatableWriteServiceImpl` (`updateDatatable` catch): why is a swallowed DDL failure now a mapped error with a new code? That is a behaviour change with no test, and the description says "No client-visible behaviour change". Separate PR please.
> * `jersey-micrometer` in `dependencies.gradle`: unrelated to config caching and a new dependency deserves its own review. Same, please split out.
> * `removeFromCache` now evicts only `<tenant>::all`. `ConfigurationDomainServiceJpa.getGlobalConfigurationPropertyData` is still `@Cacheable("configByName")` keyed `tenant + name`, and the old `@CacheEvict` here used to clear it. Where is it evicted now? In SINGLE_NODE it looks stale until the TTL. How do the two caches interact, and is the new one redundant for the hottest caller? The `propertyName` argument is now only a log line, drop it.
> * The read path returns `eclipselink.read-only` entities shared across threads for the whole TTL, and nothing stops a later caller from calling a setter. Return `toData()`, or add a test that fails if someone does.
> * `ThreadLocalContextUtil` in core now depends on the cache package. Is `reset()` guaranteed at the end of every request and job thread, otherwise the map leaks?
> * Tests run on one thread while the claim is per-thread isolation: pls add a test that thread B's `afterBegin` does not clear thread A, one that `reset()` drops the store, and one integration test that switches to SINGLE_NODE via `PUT /caches` and stays cached.
> * Smaller: `TransactionScopedCacheManager` has a single implementation and `RuntimeDelegatingCacheManager` does an `instanceof` anyway, `CONFIG_BY_NAME_CACHE_NAME` is an alias of a constant in the wrapper, mock `GlobalConfigurationProperty` is avoidable.
Thanks @galovics, all addressed:
- Datatable catch and jersey-micrometer: removed from this PR; they'll come as separate PRs.
- configByName eviction: ConfigurationDomainServiceJpa no longer has @Cacheable — it was removed in e9e31a8190 (FINERACT-1724, 2023) and the method just delegates to the wrapper, so there is a single cache and the wrapper's eviction covers it. Looking into it, though, I found a real problem in the opposite direction: CacheConfig only creates Ehcache caches for names found by the @Cacheable scan, so once the annotation was gone, SINGLE_NODE had no configByName cache and every lookup hit the DB. The transaction-bound caches are now registered explicitly, with a unit test against the real Ehcache that fails without the fix. propertyName dropped.
- Shared entities: the read path now returns a fresh GlobalConfigurationPropertyData per call; cached entities never leave the wrapper. Test added.
- reset(): every HTTP request thread resets at start and in finally (TenantAwareBasicAuthenticationFilter, OIDC/auth filters), and jobs/COB/batch workers reset too (21 call sites). Retention is bounded anyway: at most two small caches per thread, emptied on every transaction begin/completion.
- Tests: two-thread isolation, reset() drops the store, and an IT that switches to SINGLE_NODE via PUT /caches, reads, updates, verifies no stale read, and restores NO_CACHE.
- Smaller: TransactionBoundCacheManager moved to core and the interface removed (it existed only because of the module boundary); alias removed; real entities in the test.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2968: Transaction-bound cache clearing and load global configs once per transaction [fineract]
Posted by "alberto-art3ch (via GitHub)" <gi...@apache.org>.
alberto-art3ch commented on PR #6493:
URL: https://github.com/apache/fineract/pull/6493#issuecomment-6450260357
> Thanks for the update. The caching design matches the description, and most of the earlier feedback is addressed. Two things remain open, plus a rebase:
>
> 1. **The datatable error-handling change is still in the diff** (inline comment on `DatatableWriteServiceImpl`).
> 2. **Registering a PPI datatable doesn't evict the new all-properties cache** (inline comment on `addSurveyConfig`).
> 3. The PR now conflicts with develop (`FineractEntityAccessUtil`), so it needs a rebase.
Thanks @adamsaghy, all three addressed and the branch is rebased on develop.
1. **Datatable error handling**: dropped. My earlier reply said it was removed but the hunk was still in the diff, sorry for the noise. `DatatableWriteServiceImpl#updateDatatable` is now byte-identical to develop; the change will come as its own PR with a test.
2. **PPI datatable registration**: you are right, and the same gap exists in the opposite direction. `deregisterDatatable` deletes the `c_configuration` row with plain JDBC too, so after deleting a PPI datatable the configuration kept resolving from the cached snapshot until the TTL. Both paths now call `GlobalConfigurationRepositoryWrapper#removeFromCache()` right after the JDBC statement. I kept the JDBC insert rather than routing through `addSurveyConfig`: that method has no callers, creates the row without the `value = '0'` the insert sets, and maps data-integrity errors differently, so reusing it would have changed behaviour beyond the eviction.
Tests:
- Unit (`DatatableWriteServiceImplTest`): registering with `CATEGORY_PPI` evicts, registering with `CATEGORY_DEFAULT` does not touch the cache, deregistering evicts.
- Integration (`GlobalConfigurationTest`): switches to SINGLE_NODE, caches the snapshot with a read, re-registers a datatable as PPI and asserts `GET /configurations/name/{dt}` finds it, then deletes the datatable and asserts the lookup is 404. NO_CACHE is restored in a `finally`. Without the evictions the first assertion fails with 404 and the second returns the stale configuration.
3. **Rebase**: done. The conflict in `FineractEntityAccessUtil` was develop replacing the SQL in-clause with `filterVisibleToOffice_ifGlobalConfigEnabled`; resolved by keeping develop's method with the wrapper's new `GlobalConfigurationPropertyData` return type. The new `ConfigurationDomainServiceJpaTest` from FINERACT-2940 stubbed the wrapper with the entity, adjusted to the data type.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org