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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
