-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Refactor BaseHapiFhirDao.getOrCreateTag method to run in a separate thread for XA transaction compatibility #6224
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
Conversation
203354b to
bc9df1e
Compare
michaelabuckley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't create tags inline during the transaction. This will cause conflicts at commit times during mass ingestion when the create tags.
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java
Outdated
Show resolved
Hide resolved
bc9df1e to
7534eb8
Compare
7534eb8 to
1154a9f
Compare
c04f886 to
fb0577f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6224 +/- ##
============================================
+ Coverage 83.54% 83.58% +0.04%
- Complexity 27432 27560 +128
============================================
Files 1707 1716 +9
Lines 106185 106634 +449
Branches 13397 13430 +33
============================================
+ Hits 88710 89135 +425
+ Misses 11750 11740 -10
- Partials 5725 5759 +34 ☔ View full report in Codecov by Sentry. |
|
FYI @michaelabuckley all tests go green now |
michaelabuckley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. I have requested a couple more changes.
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TagDefinitionDao.java
Outdated
Show resolved
Hide resolved
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TagDefinitionDao.java
Outdated
Show resolved
Hide resolved
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TagDefinitionDao.java
Outdated
Show resolved
Hide resolved
hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningSqlR4Test.java
Outdated
Show resolved
Hide resolved
michaelabuckley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry. I've led you down an obsolete path. I forgot that we relaxed the constraint on the hfj_tag_def table. It used to be unique. But that broke last year, and we now allow duplicate definitions in hfj_tag_def. So all this complexity with the nested transaction is no longer needed.
I think your original path will work. Just commit the tag definitions as part the main transaction, and add them to the shared cache once submitted.
See #4813
Again, I apologize for asking for this complexity.
fb0577f to
25a62b2
Compare
…XA transaction compatibility
The getOrCreateTag method previously used a propagation behavior that caused issues with
XA transactions when using the PostgreSQL JDBC driver. The PGXAConnection does not support
transaction suspend/resume, which made it incompatible with the existing propagation strategy
'PROPAGATION_REQUIRES_NEW'.
This refactor changes the getOrCreateTag logic to perform a lookup/write in a new transaction
as before, but running in a separate thread, such that the main transaction is not suspended.
The result is retrieved through a future.
This change aims to improve compatibility and prevent transaction-related issues when using HAPI-FHIR with
XA transactions and PostgreSQL.
Closes hapifhir#3412
341b059 to
966509c
Compare
- Simplified tag creation by removing unnecessary transaction complexity, since we allow duplicate tags in hfj_tag_def from hapifhir#4813 - Removed redundant retry logic based on updated DB constraints
966509c to
b18c22b
Compare
jamesagnew
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really nice clean solution in the end. Thanks for all the work on it!
XA Transaction Issue with HAPI FHIR and PostgreSQL
We are experiencing issues in our HAPI FHIR application when using XA transactions, similar to those reported in issue #3412.
Problem
The PostgreSQL JDBC driver's
PGXAConnectiondoes not support suspend/resume operations, as detailed in the official documentation. This limitation causes exceptions when thePROPAGATION_REQUIRES_NEWtransaction propagation behavior is used in XA environments, since it requires suspending the current transaction and resuming it later.Proposed Solution
The solution focuses on simplifying the
TagDefinitionhandling by addressing the primary concern of managing tag creation in a concurrent environment. Previously, complex retry and nested transaction logic was necessary due to a uniqueness constraint on thehfj_tag_deftable, which prevented duplicate tag definitions. However, this constraint was relaxed, and duplicate tag definitions are now allowed, making the nested transaction and retry logic obsolete.