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 Transaction Manager Recovery #71

Merged
merged 5 commits into from
Nov 30, 2023
Merged

Support Transaction Manager Recovery #71

merged 5 commits into from
Nov 30, 2023

Conversation

gastaldi
Copy link
Member

  • This registers the Managed Connection Factory to the Recovery service if enabled.
  • Also added config options for the recovery credentials

@gastaldi gastaldi requested a review from a team as a code owner November 10, 2023 19:14
@gastaldi
Copy link
Member Author

@jhalliday can you please take a look if this makes sense?

@gastaldi
Copy link
Member Author

@vsevel in case you have recovery enabled, can you test with this PR if the transaction is correctly recovered in case of a server crash?

@vsevel
Copy link
Contributor

vsevel commented Nov 13, 2023

no. we do not have recovery enabled.

@zhfeng
Copy link
Contributor

zhfeng commented Nov 21, 2023

@gastaldi I add a crash recovery test https://github.com/zhfeng/jca-recovery-test. Only test it on JVM mode.

  • build an application
./mvnw clean package
  • start artemis server
docker run --name artemis \
  -e AMQ_USER=admin -e AMQ_PASSWORD=admin \
  -d -p 61616:61616 \
  quay.io/artemiscloud/activemq-artemis-broker

  • run the application
java -jar target/quarkus-app/quarkus-run.jar
  • send a crash message
curl -X POST http://localhost:8080/jca?name=crash
  • restart the application and you can see such logs
2023-11-21 11:19:22,476 INFO  [io.qua.iro.runtime] (jca-worker-pool-<default>-1) QIJ000001: Starting Resource Adapter <default>: ActiveMQ Artemis 2.31.2
2023-11-21 11:19:22,492 INFO  [org.acm.DummyXAResourceRecovery] (main) register DummyXAResourceRecovery
2023-11-21 11:19:22,496 INFO  [org.apa.act.art.ra.ActiveMQRALogger] (jca-worker-pool-<default>-1) AMQ151007: Resource adaptor started
2023-11-21 11:19:22,534 INFO  [io.quarkus] (main) code-with-quarkus 1.0.0-SNAPSHOT on JVM (powered by Quarkus 3.2.9.Final) started in 0.627s. Listening on: http://0.0.0.0:8080
2023-11-21 11:19:22,534 INFO  [io.quarkus] (main) Profile prod activated. 
2023-11-21 11:19:22,534 INFO  [io.quarkus] (main) Installed features: [cdi, ironjacamar, narayana-jta, resteasy, smallrye-context-propagation, vertx]
2023-11-21 11:19:32,572 INFO  [org.acm.DummyXAResourceRecovery] (Periodic Recovery) DummyXAResourceRecovery returning list of resources: [org.acme.DummyXAResource@17ad6ab0]
2023-11-21 11:19:32,577 INFO  [org.acm.DummyXAResource] (Periodic Recovery) Committing DummyXAResource
  • check the message
curl http://localhost:8080/jca

and you will get Hello crash.

Copy link
Contributor

@zhfeng zhfeng left a comment

Choose a reason for hiding this comment

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

LGTM

@gastaldi
Copy link
Member Author

I'll do a minor refactoring before merging this PR

@zhfeng
Copy link
Contributor

zhfeng commented Nov 28, 2023

@gastaldi I have some changes to add a property to disable passing XATerminator in some case. see zhfeng@af88d23

@zhfeng
Copy link
Contributor

zhfeng commented Nov 28, 2023

Is there any chance to add it?

@gastaldi
Copy link
Member Author

@zhfeng Interesting, but it doesn't make much sense to me TBH. Can you elaborate on why this is needed?

@zhfeng
Copy link
Contributor

zhfeng commented Nov 28, 2023

Well, when we did a test with DTPRA (which is used to connect to the mainframe system), it would use XATerminator to run a inboundXids recovery in a seperate thread like DTPMAIN. And this causes the some failures confilcting with quarkus-agroal in narayana recovery thread due to https://issues.redhat.com/browse/AG-227 . Now the only way to disable its recovery thread in DTPRA is disabling passing XATerminator in BootStrapContext. Yeah, this looks like a workaround and we need to fix AG-227 later. Also it could re-consider with https://issues.redhat.com/browse/JBTM-3325 in narayana side.

@gastaldi
Copy link
Member Author

I see. The best solution in this case IMHO is to change the DTPRA adapter to ignore that (maybe through a flag in the adapter), not the extension.

@gastaldi gastaldi requested a review from zhfeng November 29, 2023 15:45
@gastaldi
Copy link
Member Author

I'll do a minor refactoring before merging this PR

Refactoring done. @zhfeng can you review it before this gets merged?

- This registers the Managed Connection Factory to the Recovery service if enabled.
- Also added config options for the recovery credentials
@gastaldi gastaldi merged commit 07ebc9a into main Nov 30, 2023
1 check passed
@gastaldi gastaldi deleted the recovery branch November 30, 2023 01:44
@gastaldi
Copy link
Member Author

I think we're now ready for a 1.1.0 release. @zhfeng Should we add something else?

@zhfeng
Copy link
Contributor

zhfeng commented Nov 30, 2023

Yeah, I think we are good for a new release. The only thing I think is to add a NOTE in docs to emphasis that if running in a XA transacction, it recommend to enable recovery by using quarkus.transaction-manager.enable-recovery=true also referring to Quarkus transaction guide . Anyway, we can do this after releasing.

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.

3 participants