Add Narayana Transaction Logs QuickStart #1311

michalvavrik · 2023-08-06T22:20:12Z

closes: #1306

Ad Check list: The guide update is being worked on by docs team right now and I don't want to interfere. If this ever pass review, I'll check with @MichalMaler how to proceed.

Check list:

Your pull request:

michalvavrik · 2023-08-06T22:28:41Z

Hey @zhfeng , WDYT? I decided against your project with dummy xa resources because it feels like it is important to show automatic recovery on XA resources registered by Agroal. I can make adjustments based on your feedback.

Recovery works, but as you can see here

quarkus-quickstarts/narayana-transaction-logs-quickstart/README.md

Line 30 in 35c09de

- `curl -v localhost:8080/transaction-logs/annotation-way`

it only happens if you make at least one request using XA transactions after crashed up was restarted in order for com.arjuna.ats.internal.jta.recovery.arjunacore.XARecoveryModule to see XA resource. Without that, transaction never recovers and you can see

2023-08-06 23:56:26,715 WARN  [com.arj.ats.jta] (Periodic Recovery) ARJUNA016037: Could not find new XAResource to use for recovering non-serializable XAResource XAResourceRecord < resource:null, txid:< formatId=131077, gtrid_length=46, bqual_length=36, tx_uid=0:ffff0a0000b9:af1b:64d01657:11, node_name=quarkus-quickstart, branch_uid=0:ffff0a0000b9:af1b:64d01657:17, subordinatenodename=null, eis_name=0 >, heuristic: TwoPhaseOutcome.FINISH_OK com.arjuna.ats.internal.jta.resources.arjunacore.XAResourceRecord@55ea0fec >
2023-08-06 23:56:26,719 WARN  [com.arj.ats.jta] (Periodic Recovery) ARJUNA016038: No XAResource to recover < formatId=131077, gtrid_length=46, bqual_length=36, tx_uid=0:ffff0a0000b9:af1b:64d01657:11, node_name=quarkus-quickstart, branch_uid=0:ffff0a0000b9:af1b:64d01657:13, subordinatenodename=null, eis_name=0 >

I presume it is not a bug, but rather something I am missing? Anyway, feedback is welcome and I think even with that additional request, this quickstarts fill its purpose.

also cc @mmusgrov (review welcomed)

zhfeng · 2023-08-07T00:50:03Z

Thanks @michalvavrik and it smells like a regression bug.

IIRC, it should work before. And can you check it with Quarkus 3.2 ?

I think there are some changes with quarkus-agroal in the main branch and DataSource looks like not initializing at the application startup.

see https://github.com/quarkusio/quarkus/blob/main/extensions/agroal/runtime/src/main/java/io/quarkus/agroal/runtime/AgroalRecorder.java#L28-L30

If that is the case, please can you open an issue on quarkus?

michalvavrik · 2023-08-07T10:31:17Z

Hey @zhfeng , fun fact, it only works with 999-SNAPSHOT, when I run app with 3.2.x.Final, app starts to fail

2023-08-07 12:18:22,356 FATAL [com.arj.ats.arjuna] (main) ARJUNA012260: Received exception for com.arjuna.ats.internal.arjuna.objectstore.jdbc.accessors.DirectDataSourceJDBCAccess:quarkus_JBossTSTxTable: java.lang.NullPointerException: Cannot invoke "io.quarkus.arc.ArcContainer.instance(java.lang.Class, java.lang.annotation.Annotation[])" because the return value of "io.quarkus.arc.Arc.container()" is null
	at io.quarkus.narayana.jta.runtime.QuarkusDataSource.getDataSource(QuarkusDataSource.java:29)
	at io.quarkus.narayana.jta.runtime.QuarkusDataSource.getConnection(QuarkusDataSource.java:38)
	at com.arjuna.ats.internal.arjuna.objectstore.jdbc.accessors.DirectDataSourceJDBCAccess.getConnection(DirectDataSourceJDBCAccess.java:27)
	at com.arjuna.ats.internal.arjuna.objectstore.jdbc.JDBCStore.initImple(JDBCStore.java:235)
	at com.arjuna.ats.internal.arjuna.objectstore.jdbc.JDBCStore.<init>(JDBCStore.java:213)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480)
	at com.arjuna.common.internal.util.ClassloadingUtility.loadAndInstantiateClass(ClassloadingUtility.java:129)
	at com.arjuna.ats.arjuna.objectstore.StoreManager.initStore(StoreManager.java:152)
	at com.arjuna.ats.arjuna.objectstore.StoreManager.getActionStore(StoreManager.java:111)
	at com.arjuna.ats.arjuna.objectstore.StoreManager.getRecoveryStore(StoreManager.java:68)
	at com.arjuna.ats.internal.arjuna.recovery.AtomicActionRecoveryModule.<init>(AtomicActionRecoveryModule.java:67)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
	at java.base/java.lang.reflect.ReflectAccess.newInstance(ReflectAccess.java:128)
	at java.base/jdk.internal.reflect.ReflectionFactory.newInstance(ReflectionFactory.java:347)
	at java.base/java.lang.Class.newInstance(Class.java:645)
	at com.arjuna.common.internal.util.ClassloadingUtility.loadAndInstantiateClass(ClassloadingUtility.java:135)
	at com.arjuna.common.internal.util.ClassloadingUtility.loadAndInstantiateClassesWithInit(ClassloadingUtility.java:192)
	at com.arjuna.ats.arjuna.common.RecoveryEnvironmentBean.getRecoveryModules(RecoveryEnvironmentBean.java:476)
	at com.arjuna.ats.internal.arjuna.recovery.PeriodicRecovery.loadModules(PeriodicRecovery.java:888)
	at com.arjuna.ats.internal.arjuna.recovery.PeriodicRecovery.<init>(PeriodicRecovery.java:121)
	at com.arjuna.ats.internal.arjuna.recovery.RecoveryManagerImple.<init>(RecoveryManagerImple.java:107)
	at com.arjuna.ats.arjuna.recovery.RecoveryManager.<init>(RecoveryManager.java:479)
	at com.arjuna.ats.arjuna.recovery.RecoveryManager.manager(RecoveryManager.java:132)
	at com.arjuna.ats.arjuna.recovery.RecoveryManager.manager(RecoveryManager.java:112)
	at io.quarkus.narayana.jta.runtime.NarayanaJtaRecorder.lambda$handleShutdown$6(NarayanaJtaRecorder.java:141)
	at io.quarkus.runtime.StartupContext.runAllInReverseOrder(StartupContext.java:84)

However you were right, it goes down to the datasource initialization. I've added following workaround that fixed problem:

package org.acme.quickstart;

import io.agroal.api.AgroalDataSource;
import io.quarkus.runtime.StartupEvent;
import jakarta.enterprise.context.ApplicationScoped;
import jakarta.enterprise.event.Observes;
import jakarta.inject.Inject;
import jakarta.inject.Named;

@ApplicationScoped
public class StartupService {

    @Inject
    AgroalDataSource dataSource;

    @Named("other-ds")
    @Inject
    AgroalDataSource otherDataSource;

    @Named("object-store-ds")
    @Inject
    AgroalDataSource objectStoreDataSource;

    public void initDataSources(@Observes StartupEvent event) {
        // force init of proxied beans
        dataSource.getMetrics();
        otherDataSource.getMetrics();
        objectStoreDataSource.getMetrics();
    }

}

Thanks! I'll open issue, your quick response is well appreciated.

michalvavrik · 2023-08-09T11:13:10Z

Upstream issue is now resolved, so I adjusted quickstart bit. Should be ready for review. cc @zhfeng

michalvavrik · 2023-08-09T13:14:31Z

hibernate-search-orm-elasticsearch-quickstart failure unrelated, now it happens twice in row, so I won't re-trigger build again.

zhfeng

LGTM !

MichalMaler · 2023-08-10T07:49:55Z

Good cooperation @michalvavrik and @zhfeng !
@zhfeng Pleaes, when you will have a minute, I sent you a message on Zullip. Please, have a look.
Cheers!

michalvavrik · 2023-08-16T08:36:52Z

I rebased on current development. CI fails on "Cache maven Repository", so I doubt it can be related.

michalvavrik · 2023-08-18T09:16:50Z

Maybe @yrodiere could have a little more time (and since he reviewed upstream transaction logs PR) than @gsmet ?

yrodiere

Hi. Thanks for working on this. I'll give my review, though I may be lacking some context as I found a few things quite unexpected for a quickstart... Hope the review is not too irrelevant.

yrodiere · 2023-08-21T12:12:35Z

narayana-transaction-logs-quickstart/docker-compose.yml

+services:
+  postgres:
+    container_name: narayana-transaction-logs-database
+    image: "postgres:latest"
+    restart: always
+    volumes:
+      - ./init-script.sql:/docker-entrypoint-initdb.d/init-script.sql
+    command: "--max_prepared_transactions=100"
+    environment:
+      POSTGRES_USER: "quarkus_test"
+      POSTGRES_PASSWORD: "quarkus_test"
+      POSTGRES_DB: "narayana_transaction_logs_db"
+    ports:
+      - "5432:5432"


Couldn't this just be configured as a dev service?

FWIW there are options to customize dev mode, in particular:

the command: https://quarkus.io/guides/databases-dev-services#quarkus-datasource-config-group-dev-services-build-time-config_quarkus.datasource.devservices.command

volumes: https://quarkus.io/guides/databases-dev-services#mapping-volumes-into-dev-services-for-database

It feels weird that we boast about Quarkus being easy to use in dev mode everywhere, and then we force people to use docker-compose in a quickstart...

yrodiere · 2023-08-21T12:13:23Z

narayana-transaction-logs-quickstart/init-script.sql

+CREATE TABLE quarkus_jbosststxtable (statetype integer not null, hidden integer not null, typename character varying(255) not null, uidstring character varying(255) not null, objectstate bytea);
+CREATE TABLE quarkus_jbosststxtable_historical_data AS SELECT * FROM quarkus_jbosststxtable;
+CREATE OR REPLACE FUNCTION object_store_historical_data() RETURNS TRIGGER AS '
+    BEGIN
+        INSERT INTO quarkus_jbosststxtable_historical_data(statetype, hidden, typename, uidstring, objectstate) VALUES (NEW.statetype, NEW.hidden, NEW.typename, NEW.uidstring, NEW.objectstate);
+        RETURN NULL;
+    END;
+' LANGUAGE plpgsql;
+CREATE TRIGGER object_store_historical_data_trigger AFTER INSERT ON quarkus_jbosststxtable FOR EACH ROW EXECUTE FUNCTION object_store_historical_data();
+CREATE TABLE audit_log (id BIGSERIAL PRIMARY KEY, message VARCHAR(10), datasource VARCHAR(40));
+CREATE TABLE example (data VARCHAR(6) NOT NULL);


For the quickstart at least, can't we rely on automatic creation? I thought I saw this feature somewhere...

yrodiere · 2023-08-21T12:18:27Z

...a-transaction-logs-quickstart/src/main/java/org/acme/quickstart/TransactionLogsResource.java

+    @GET
+    @Produces(MediaType.TEXT_PLAIN)
+    @Transactional
+    @Path("/annotation-way")
+    public boolean annotationWay(@RestQuery boolean rollback) throws SystemException {
+        var result = makeTransaction();
+        if (rollback) {
+            transactionManager.getTransaction().setRollbackOnly();
+        }
+        return result;
+    }
+
+    @GET
+    @Produces(MediaType.TEXT_PLAIN)
+    @Path("/static-transaction-manager-way")
+    public boolean staticTransactionManagerWay(@RestQuery boolean rollback) throws SystemException, NotSupportedException,
+            HeuristicRollbackException, HeuristicMixedException, RollbackException {
+        com.arjuna.ats.jta.TransactionManager.transactionManager().begin();
+        var result = makeTransaction();
+        if (rollback) {
+            com.arjuna.ats.jta.TransactionManager.transactionManager().rollback();
+        } else {
+            com.arjuna.ats.jta.TransactionManager.transactionManager().commit();
+        }
+        return result;
+    }
+
+    @GET
+    @Produces(MediaType.TEXT_PLAIN)
+    @Path("/injected-transaction-manager-way")
+    public boolean injectedTransactionManagerWay(@RestQuery boolean rollback) throws SystemException, NotSupportedException,
+            HeuristicRollbackException, HeuristicMixedException, RollbackException {
+        transactionManager.begin();
+        var result = makeTransaction();
+        if (rollback) {
+            transactionManager.rollback();
+        } else {
+            transactionManager.commit();
+        }
+        return result;
+    }
+
+    @GET
+    @Produces(MediaType.TEXT_PLAIN)
+    @Path("/injected-user-transaction-way")
+    public boolean injectedUserTransactionWay(@RestQuery boolean rollback) throws SystemException, NotSupportedException,
+            HeuristicRollbackException, HeuristicMixedException, RollbackException {
+        userTransaction.begin();
+        var result = makeTransaction();
+        if (rollback) {
+            userTransaction.rollback();
+        } else {
+            userTransaction.commit();
+        }
+        return result;
+    }
+
+    @GET
+    @Produces(MediaType.TEXT_PLAIN)
+    @Path("/static-user-transaction-way")
+    public boolean staticUserTransactionWay(@RestQuery boolean rollback) throws SystemException, NotSupportedException,
+            HeuristicRollbackException, HeuristicMixedException, RollbackException {
+        com.arjuna.ats.jta.UserTransaction.userTransaction().begin();
+        var result = makeTransaction();
+        if (rollback) {
+            com.arjuna.ats.jta.UserTransaction.userTransaction().rollback();
+        } else {
+            com.arjuna.ats.jta.UserTransaction.userTransaction().commit();
+        }
+        return result;
+    }


I'm not sure showing uses of UserTransaction has value in a quickstart, in particular when our documentation calls that the "legacy approach".

Using the transaction manager directly also seems a bit odd.

IMO if we want to showcase multiple ways of handling transactions, we should go with @Transaction and QuarkusTransaction.

yrodiere · 2023-08-21T12:20:39Z

...a-transaction-logs-quickstart/src/main/java/org/acme/quickstart/TransactionLogsResource.java

+    public void deleteObjectStoreHistoricalData() {
+        // delete content of database table where we back up each insert into JDBC object store
+        // it is only necessary for our tests so that we can run every test against clean table


Maybe call this testCleanup and make it a @Startup method, to avoid any misunderstanding and remove confusing statements from tests?

yrodiere · 2023-08-21T12:24:18Z

...saction-logs-quickstart/src/main/java/org/acme/quickstart/recovery/CrashingXAConnection.java

+public class CrashingXAConnection extends PGXAConnection {
+
+    private static final Logger LOG = Logger.getLogger(CrashingXAConnection.class);
+    private final PGXAConnection delegate;
+    private volatile InstanceHandle<RoutingContext> routingContextInstanceHandle;
+
+    CrashingXAConnection(PGXAConnection delegate, BaseConnection connection) throws SQLException {
+        super(connection);
+        this.delegate = delegate;
+    }


This is a quickstart, supposed to give people an example of how to use Narayana in their Quarkus application.

So, I don't think this kind of test fixture has its place here. Why would someone need a crashing connection in their project?

I get that we need to demonstrate that transactions work correctly somehow, but... isn't there a simpler approach, one that we could find in an actual application? Wouldn't it be enough to throw some exception during a transaction started with @Transaction/QuarkusTransaction?

yrodiere · 2023-08-21T12:24:55Z

...ction-logs-quickstart/src/main/java/org/acme/quickstart/recovery/CrashingPGXADataSource.java

+import java.sql.Connection;
+import java.sql.SQLException;
+
+public class CrashingPGXADataSource extends PGXADataSource {


Same comment as for CrashingXAConnection, I'm not sure a quickstart is the right place for this kind of things.

yrodiere · 2023-08-21T12:26:00Z

narayana-transaction-logs-quickstart/src/main/resources/application.properties

+# configure 2 datasources with enabled XA transactions
+quarkus.datasource.db-kind=postgresql
+quarkus.datasource.username=quarkus_test
+quarkus.datasource.password=quarkus_test
+quarkus.datasource.jdbc.url=jdbc:postgresql://localhost/narayana_transaction_logs_db
+quarkus.datasource.jdbc.transactions=xa
+quarkus.datasource.jdbc.driver=org.acme.quickstart.recovery.CrashingPGXADataSource
+quarkus.datasource.other-ds.jdbc.transactions=${quarkus.datasource.jdbc.transactions}
+quarkus.datasource.other-ds.db-kind=${quarkus.datasource.db-kind}
+quarkus.datasource.other-ds.username=${quarkus.datasource.username}
+quarkus.datasource.other-ds.password=${quarkus.datasource.password}
+quarkus.datasource.other-ds.jdbc.url=${quarkus.datasource.jdbc.url}
+quarkus.datasource.other-ds.jdbc.driver=${quarkus.datasource.jdbc.driver}
+
+# configure transaction manager to use JDBC object store and enable automatic recovery
+quarkus.transaction-manager.node-name=quarkus-quickstart
+quarkus.transaction-manager.object-store.type=jdbc
+quarkus.transaction-manager.object-store.datasource=object-store-ds
+quarkus.transaction-manager.enable-recovery=true
+
+# datasource used for the JDBC object store must have disabled transactions
+quarkus.datasource.object-store-ds.jdbc.transactions=disabled
+quarkus.datasource.object-store-ds.db-kind=${quarkus.datasource.db-kind}
+quarkus.datasource.object-store-ds.username=${quarkus.datasource.username}
+quarkus.datasource.object-store-ds.password=${quarkus.datasource.password}
+quarkus.datasource.object-store-ds.jdbc.url=${quarkus.datasource.jdbc.url}


This config can be simplified a lot if you rely on dev services (see my other comment on that topic).

yrodiere · 2023-08-21T12:28:04Z

narayana-transaction-logs-quickstart/README.md

+- build application: 
+  - `./mvnw clean package -DskipTests -DskipITs`
+- remove previous database volumes, so that we start with a clean sheet:
+  - `docker-compose down --volumes`
+- start database: 
+  - `docker compose up`
+- wait until container has started and is ready to accept connections
+- start application: 
+  - `java -jar ./target/quarkus-app/quarkus-run.jar`


this could be summed up as ./mvnw clean compile quarkus:dev if you used dev services.

michalvavrik · 2024-09-23T08:52:57Z

This PR needs to be adapted to changes in Quarkus and Quakurs Quickstarts that happen in between. I'll do it later this week, address, Yoann comments and reopen it.