Fix caching mechanism for Sagas #2517
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…perly, as well as leading to NPE
CodeDrivenMitch
requested review from
abuijze,
a team,
gklijs and
smcvb
and removed request for
a team
CodeDrivenMitch
added
Type: Bug
gklijs
reviewed
Dec 15, 2022
messaging/src/main/java/org/axonframework/common/caching/Cache.java
Outdated
Show resolved
Hide resolved
- Fix indentations - Solve warnings - Make Cache#computeIfAbsent a default implementation, throwing an unsupported exception when not implemented. #2517
smcvb
previously requested changes
Dec 15, 2022
abuijze
reviewed
Dec 15, 2022
messaging/src/main/java/org/axonframework/common/caching/Cache.java
Outdated
Show resolved
Hide resolved
Add integration tests for EhCache and WeakReferenceCache, using a common test suite for validation. #2517
abuijze
requested changes
Dec 16, 2022
messaging/src/main/java/org/axonframework/common/caching/Cache.java
Outdated
Show resolved
Hide resolved
…exception if the value supplier returns null to differentiate between faulty cache and faulty saga stores
Provide working default implementation of computeIfAbsent. As, if users do have their own implementation of the Cache, their migration to a more recent version of Axon Framework would horribly break. #2517
- Consistently use newValue as the result of the valueSupplier - Suppress warnings #2517
- Move common used fields to constants - Remove manual cache clearing, as that's a job of the IT implementations to define when these are cleared. - Maintain references to the associations set, to ensure the weak reference implementation doesn't fail on the missing associations - Validate whether the saga cache entry is present before verifying the contents, instead of an assertion. This it to cover for cache implementation that may have cleared out the entries between the (bulk) updates and validation. As this is a perfectly valid scenario, just don't validate the cache if it's not there instead of breaking the test case. - Suppress unimportant warnings #2517
smcvb
dismissed
their stale review
As I'm working on these changes myself, my review may be dismissed.
By introducing the default implementation on the Cache interface, we can get rid of the concrete implementation on the EhCache and JCache adapters. #2517
abuijze
reviewed
Dec 19, 2022
messaging/src/main/java/org/axonframework/common/caching/WeakReferenceCache.java
Outdated
Show resolved
Hide resolved
messaging/src/main/java/org/axonframework/common/caching/WeakReferenceCache.java
Outdated
Show resolved
Hide resolved
messaging/src/main/java/org/axonframework/common/caching/JCacheAdapter.java
Outdated
Show resolved
Hide resolved
messaging/src/main/java/org/axonframework/common/caching/EhCacheAdapter.java
Outdated
Show resolved
Hide resolved
- Reuse existingValue instead of retrieving it from the entry. - Use ObjectUtils.getOrDefault #2517
smcvb
approved these changes
Dec 19, 2022
|
As discussed offline, we (Mitchell, Allard, me) approve of this current solution. |
smcvb
added
Status: Resolved
|
SonarCloud Quality Gate failed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR expands the
Cacheinterface with acomputeIfAbsent(Object key, Supplier<T> valueSupplier)method, which lazily fetches data when data is not found in the cache.The current implementation always queries the Saga store, and puts it in the cache afterwards. The NPE experienced by a customer might be because of eventual consistency in the cache. We
getit, right after weputit, so it might not have replicated yet (this depends on the configuration) and produce anullvalue.This is not the case when using the new method. This will check the cache and only query for the value when necessary. In addition, it will return the produced value instead of the value in the cache, ensuring there actually is a value upon return.