We should also change the quickstart to use Postgres (initially we can just use our server as described here https://github.com/jbosstm/narayana/blob/master/qa/config/jdbc_profiles/default/JDBCProfiles#L66 and change to use your framework later)
We should change the DBCP we use to the one provided by org.apache.tomcat:tomcat-dbcp:9.0.7
karm - what do you think about raising the PR here: Comparing jbosstm:master...Karm:narayana-tomcat-ts · jbosstm/narayana · GitHub though it looks like it needs a rebase. Also as you say it uses docker which our slaves don't but perhaps the default run could be hard coded to our Postgres instance here: narayana/JDBCProfiles at master · jbosstm/narayana · GitHub
Sorry - I see it has dballocator still my page must not have refreshed fully
zhfeng I have just tried some initial commits on the Tomcat 9 change: JBTM-3008 Upgrade to Tomcat 9 by tomjenkinson · Pull Request #1299 · jbosstm/narayana · GitHub and JBTM-3008 Upgrade to Tomcat 9 by tomjenkinson · Pull Request #223 · jbosstm/quickstart · GitHub
They may well not work so I put them in the Narayana github branches so we can collab easily.
If they do work lets update the quickstart to point at our PostGres instead of H2 narayana/JDBCProfiles at master · jbosstm/narayana · GitHub
tomjenkinson I merged the tomcat-jta tests with what karm prepared (I hope in the right way now). Tests are passing for me. Currently I only worry about the leaking connections as Karm mentioned in at the ([JBTM-3005] Add a quickstart showing the Tomcat and DBCP2 - JBoss Issue Tracker ) and we've discussed at (JBTM-3009 Update to add transactional datasource factory in tomcat-jta by zhfeng · Pull Request #1297 · jbosstm/narayana… ).
My configuration changes are here (as mentioned there is an issue with the class loading from lib and war/lib).
For my testing I used local docker but the postgre connection could be configured in context.xml differently.
If somebody would like to test these are my steps
docker run -p 5432:5432 --rm -ePOSTGRES_USER=test -e POSTGRES_PASSWORD=test postgres:9.4 -c max-prepared-transactions=110 -c log-statement=all -c max_connections=20 # full postgresql connection settings by Karm is at https://github.com/Karm/narayana/blob/narayana-tomcat-ts/tomcat/tomcat-jta/src/test/java/org/jboss/narayana/tomcat/jta/integration/utils/PostgresContainerAllocator.java#L201 git clone https://github.com/ochaloup/narayana.git -b dbcp2 cd narayana ./build.sh clean install -Pcommunity -DskipTests -Didlj-enabled=true cd tomcat/tomcat-jta # download and unzip apache tomcat version 9.0.7 export CATALINA_HOME=... sed -i 's|</tomcat-users>|<user username="test" password="test" roles="manager-script"/>\n</tomcat-users>|' $CATALINA_HOME/conf/tomcat-users.xml cp ~/.m2/repository/org/jboss/spec/javax/transaction/jboss-transaction-api_1.2_spec/1.0.0.Final/jboss-transaction-api_1.2_spec-1.0.0.Final.jar $CATALINA_HOME/lib/ mvn clean verify -Parq-tomcat -Dit.test=BaseITCase
One more point: I was still unsure why Karm's test was failing (https://issues.jboss.org/browse/JBTM-3005?focusedCommentId=13558264#comment-13558264) and I can say that's because recovery manager was not enough time to finish the in-doubt transactions (initially taken from the quickstart).
By me I would summarized it as:
* Our basic tests pass and it seems the basis of the integration works.
* I think new tests can be added to the tomcat-jta in the Narayana project for getting better coverage and to see if integration works fine.
* We should consider running tomcat-jta against our databases at the CI cluster. I think the generator of the context.xml that Karm prepared could be useful in that. We can then just with a parameter (parameters) change what database should be run and where. But maybe it's not so urgent now.
* There could be possibly some more work at the datasource factory for ensuring not leaking connection and maybe finding better way of setting up the pool properties.
Thanks for the research ochaloup I managed to clean a few things up in my fork too: JBTM-3008 Upgrade to Tomcat 9 by tomjenkinson · Pull Request #1299 · jbosstm/narayana · GitHub
I see more that Karm did not change the BaseITTest fundamentally and his changes were rather to made it work with Postgres via the allocator process so I agree that the basic tests pass.
I will change (similar to you ) the datasource in the tests context.xml to be a postgres one, though I will use our CI node rather than docker as our Jenkins slaves don't have permission to launch docker containers that I am aware of?
Currently I only worry about the leaking connections as Karm mentioned in at the ([JBTM-3005] Add a quickstart showing the Tomcat and DBCP2 - JBoss Issue Tracker ) and we've discussed at (JBTM-3009 Update to add transactional datasource factory in tomcat-jta by zhfeng · Pull Request #1297 · jbosstm/narayana… ).
I share the concern as to that being a potential source of a leak so raised this: https://github.com/jbosstm/narayana/pull/1300[JBTM-3012] Cache recovery connection in Tomcat integration - JBoss Issue Tracker (PR https://github.com/jbosstm/narayana/pull/1300)
I seem to be getting a strange error on my machine complaining that TransactionManager cannot be found in fact. I am not sure if it relates to my changes (i.e. the upgrade to Tomcat 9)
13-Apr-2018 14:42:45.865 INFO [http-nio-8080-exec-3] org.apache.catalina.startup.HostConfig.deployWAR Deploying web application archive [C:\Users\still\Documents\jbosstm\narayana\apache-tomcat-9.0.7\webapps\test.war]
13-Apr-2018 14:42:48.472 WARNING [http-nio-8080-exec-3] org.apache.catalina.core.NamingContextListener.addResource Failed to register in JMX: [javax.naming.NameNotFoundException: Name [TransactionManager] is not bound in this Context. Unable to find [TransactionManager].]
13-Apr-2018 14:42:48.503 INFO [http-nio-8080-exec-3] org.apache.jasper.servlet.TldScanner.scanJars At least one JAR was scanned for TLDs yet contained no TLDs. Enable debug logging for this logger for a complete list of JARs that were scanned but no TLDs were found in them. Skipping unneeded JARs during scanning can improve startup time and JSP compilation time.
13-Apr-2018 14:42:48.708 WARNING [http-nio-8080-exec-3] org.jboss.narayana.tomcat.jta.NarayanaJtaServletContextListener.initNodeIdentifier Node identifier was not set. Setting it to the default value: 1
13-Apr-2018 14:42:48.719 SEVERE [http-nio-8080-exec-3] org.apache.catalina.core.StandardContext.startInternal One or more listeners failed to start. Full details will be found in the appropriate container log file
13-Apr-2018 14:42:48.720 SEVERE [http-nio-8080-exec-3] org.apache.catalina.core.StandardContext.startInternal Context [/test] startup failed due to previous errors
13-Apr-2018 14:42:48.895 INFO [http-nio-8080-exec-3] com.arjuna.ats.arjuna.recovery.TransactionStatusManager.start ARJUNA012170: TransactionStatusManager started on port 55988 and host 127.0.0.1 with service com.arjuna.ats.arjuna.recovery.ActionStatusService
karm it looks like the Tomcat 9.0.7 needs the JTA library adding to it because of the "managed" DBCP package referring to the classes. If we add the JTA jar to the web app the classloader gets a bit screwed up. Ondra seems to have spotted this too and for now I am scripting the same: testing with dbcp2 from tomcat 9.0 · ochaloup/narayana@a1bd77f · GitHub but eventually we should try to have the JTA library in Tomcat so the user doesn't have to copy to Tomcat lib folder.
I have wrapped all this into one PR now but three issues resolved in it (hopefully);
1. upgraded to Tomcat 9
2. Changed BaseITTest to use Postgres
3. Eliminated connection leak in the recovery registration for the TransactionalDataSourceFactory
Let's see how the PR goes...
mbabacek I think I was pinging you on the wrong ID sorry. Anyway the PR has passed so once zhfeng and ochaloup can verify the review it should mean we are happy that it is all working on Tomcat 9 with Postgres and Tomcat DBCP which should mean that the pooling is working.
Once it is merged you could try a rebase of your testsuite changes and then we can go from there but do note my comment above about how our CI slaves don't have docker (yet) and so ideally it would per default just use our CI postgres.