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

Support multiple JpaDataStore #2998

Merged
merged 8 commits into from
Jun 4, 2023
Merged

Support multiple JpaDataStore #2998

merged 8 commits into from
Jun 4, 2023

Conversation

justin-tay
Copy link
Collaborator

In order to support multiple transactional JpaDataStore in Spring the MultiplexManager needed to be modified to process transactions last-in-first-out instead of first-in-first-out. After making this modification, a test was failing. The test was testing that the previously committed transaction on a HashMapDataStore was being reversed by the MultiplexManager. Reversing the order meant that the transaction on the HashMapDataStore wasn't even being commited, however the data was still modified. The objects retrieved from the HashMapDataStore are the same reference from the store, so any modifications to the store actually already modifies the data in the underlying store.

In order to fix this the following things were done to the HashMapDataStore.

  • A ReadWriteLock was introduced so write transactions need an exclusive lock when making modifications
  • A rollback implementation similar to what the MultiplexManager does to reverse the transaction

The new HashMapDataStoreTest#testShouldNotReadDirtyData test demonstrates the issue and the fix.

Description

  • Spring's JtaTransactionManager is now supported
  • A @EnableJpaDataStore annotation is added to make it easy to add and configure multiple JpaDataStore. Allows configuring the entity manager, transaction manager or the managed classes.
  • Introduced a DataStoreBuilderCustomizer and JpaDataStoreRegistrationsBuilderCustomizer to allow customization of the autoconfiguration without needed to totally copy and paste the existing codes.
  • MultiplexManager now processes transactions last-in-first-out.
  • HashMapDataStore modified for data consistency.

Motivation and Context

Makes it possible to have multiple transactional JpaDataStore in the same transaction. Makes the configuration of JpaDataStore easier.

How Has This Been Tested?

Tested with unit and integration tests.

In particular the following were tested for Spring

  • 2 x JpaTransactionManager and 2 x EntityManagerFactory
  • 1 x JtaTransactionManager and 2 x EntityManagerFactory

The Atomikos JtaTransactionManager was used for the test.

License

I confirm that this contribution is made under an Apache 2.0 license and that I have the authority necessary to make this contribution on behalf of its copyright owner.

Copy link
Member

@aklish aklish left a comment

Choose a reason for hiding this comment

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

Looked over elide-core only so far.

@@ -151,6 +171,7 @@ public Object loadObject(EntityProjection projection, Serializable id, RequestSc

synchronized (dataStore) {
Map<String, Object> data = dataStore.get(projection.getType());
cacheForRollback(projection.getType(), data);
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove the synchronized blocks in this class now that there is a read/write lock on the transaction

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have removed the synchronized blocks.

* @param target the bean to copy to
* @param cls the class
*/
public static void copyProperties(Object source, Object target, Type<?> cls) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice improvements over the clone in MultiplexWriteTransaction.

@justin-tay
Copy link
Collaborator Author

I think the build error is transient. I tried running the failing example.tests.AsyncTest.testExportDynamicModel test locally a few times and cannot reproduce it.

@aklish aklish merged commit 855ab18 into yahoo:master Jun 4, 2023
@justin-tay justin-tay deleted the jpastores branch July 18, 2023 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants