Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saga Caching Enhancements #2531

Merged
merged 19 commits into from Dec 29, 2022
Merged

Saga Caching Enhancements #2531

merged 19 commits into from Dec 29, 2022

Conversation

smcvb
Copy link
Member

@smcvb smcvb commented Dec 23, 2022

This pull request does a couple of things:

  1. It enhances the CachedSaga instance to construct N associations. This is done, as the number of associations may impact the cache invocations
  2. It adjusts the CachingIntegrationTestSuite to use a TrackingEventProcessor instead of a SubscribingEventProcessor. This adjustments lets the IT mirror Saga Caching use cases more closely, as we recommend the use of a StreamingEventProcessor (like the TrackingEventProcessor) to our users. Furthermore, as this makes event handling asynchronous, the validation process is adjusted to wait for the expected amount of events to be handled.
  3. It adjusts the CachingIntegrationTestSuite to use a custom Cache.EntryListener to validate the Cache is accessed as expected.
  4. It ensures the WeakReferenceCachce#computeIfPresent invokes the Cache.EntryListener.
  5. The outcome of the Cache#computeIfAbsent lambda invocation in the CachingSagaStore to retrieve associations is wrapped into a Collections#synchronizedSet. This resolves any ConcurrentModificationExceptions that may occur upon invocation of the CachingSagaStore#findSagas method, as the set within the Cache is replaced by a thread-safe variant.
  6. It adds a harsher concurrency test to the CachingSagaStoreTest, validating the ConcurrentModificationException does not occur anymore.

Instead of using the SubscribingEventProcessor, the test suite should
use a StreamingEventProcessor, like the TEP. Doing so, it will more
closely mirror reality for our users.

#enhancement/caching-saga-it
Wrap result of cache retrieval in a new HashSet. Otherwise, there's a
window of opportunity that the cached result Set is adjusted while the
SagaRepository uses the contents to add to the found sagas result

#enhancement/caching-saga-it
@smcvb smcvb added Type: Bug Use to signal issues that describe a bug within the system. Priority 1: Must Highest priority. A release cannot be made if this issue isn’t resolved. Status: In Progress Use to signal this issue is actively worked on. labels Dec 23, 2022
@smcvb smcvb added this to the Release 4.6.3 milestone Dec 23, 2022
@smcvb smcvb self-assigned this Dec 23, 2022
smcvb and others added 17 commits December 27, 2022 15:53
Add more association adjustments

#enhancement/caching-saga-it
Make WeakReferenceCache#computeIfPresent use EntryListeners, as that's
intended for usage of the WeakReferenceCache.

#enhancement/caching-saga-it
Use a Cache.EntryListener to validate instead of the Cache directly, as
that provides additional certainty that the Cache hasn't been adjusted
during the given/when phases of the tests. Doing so, we make the test
cases more resilient.

#enhancement/caching-saga-it
Fix wait times to be less extensive

#enhancement/caching-saga-it
… the delegate in a synchronised set to prevent concurrency issues leading to a ConcurrentModificationException in some cases.
… check whether value is still not garbage collected or cleaned
…it-morlack

Proposed changes to caching saga fix
Remove redundant empty lines

#2531
Adjust indentation and naming of test case for clarity

#2531
Due to the Saga default TEP configuration that isn't impacted by
configuring a default TEP config for al TEP's, the provided
configuration did not have any impact at all.

#2531
As there's a window of opportunity that a Saga is searched for right
after it's removed, the removed check may currently fail from time to
time. As this deviation isn't important for the test cases, we can get
rid of clearing out the removed set when an entry is added. Added,
clean-up the validator a bit further for clarity.

#2531
Increase timeouts for resiliency, since busier systems may fail due to
the current timeouts.

#2531
Replace await-call for assertTrue. The await-call was included to
incorrectly resolve the occurrence where a saga was found right after
deletion, causing it to turn back up in the created set, thus clearing
out the removed set. With the adjustments in the EntryListenerValidator,
 this predicament no longer occurs though.

#2531
Adjust indentation

#2531
@sonarcloud
Copy link

sonarcloud bot commented Dec 28, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

80.0% 80.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@CodeDrivenMitch CodeDrivenMitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@smcvb smcvb merged commit 1746fb7 into axon-4.6.x Dec 29, 2022
@smcvb smcvb deleted the enhancement/caching-saga-it branch December 29, 2022 16:11
@smcvb smcvb added Status: Resolved Use to signal that work on this issue is done. and removed Status: In Progress Use to signal this issue is actively worked on. labels Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority 1: Must Highest priority. A release cannot be made if this issue isn’t resolved. Status: Resolved Use to signal that work on this issue is done. Type: Bug Use to signal issues that describe a bug within the system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants