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
JBPM-7266 Configure jBPM unit tests to use connection pooling #1217
Conversation
this is actually misconfiguration I'd say - since there is jta-data-source given in persistence.xml username and password in properties should anyway be ignored so I'd say let's remove it and rely only on data source instead. |
+1, will remove once we have something working |
@rsynek how are we doing with this one? do we wait for JWS team at the moment? |
@mswiderski just FYI @rsynek is on vacation till the ond of this week. |
@mswiderski yes, here is the issue I opened for this purpose https://issues.jboss.org/browse/JWS-1028 |
jenkins retest this |
jenkins retest this |
@rsynek can you try my commit to fix the interposed synchonization |
4305bb8
to
8e50eb1
Compare
Can one of the admins verify this patch? |
jenkins retest this |
xaDataSource.getClass().getMethod("setPassword", new Class[]{String.class}).invoke(xaDataSource, password); | ||
xaDataSource.getClass().getMethod("setUser", new Class[]{String.class}).invoke(xaDataSource, username); | ||
} | ||
|
||
if (!(className.startsWith("com.ibm.db2") || className.startsWith("com.sybase"))) { |
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.
Shouldn't this be rather else if
then?
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.
There is already else
branch on line https://github.com/rsynek/jbpm/blob/b5feebfc6fce286b5e66ef07b6fb80963bf0ec0b/jbpm-test-util/src/main/java/org/jbpm/test/util/PoolingDataSource.java#L156 being currently executed for H2 too. I will think about making this part more readable - right now I wanted to see how many failures this fix eliminates.
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.
Ok, that's fair.
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.
A bit of refactoring we talked about - in the target (not yet used) component web-servers/narayana-tomcat#1
Can one of the admins verify this PR? Comment with 'ok to test' to start the build. |
jenkins retest this |
Failed probably due to jenkins being restarted. |
jenkins retest this |
The single failure is probably a random one; locally the test works. |
@rsynek so what’s the next step for it? |
@mswiderski the next step is to change the tomcat-dbcp 9.0-SNAPSHOT to stable version, once it's ready. I will also request a build of tomcat-jta with [1] and remove the boiler plate code from jbpm-test-util. This second step is not necessary, but nice to have. |
jenkins retest this |
…oOpTransactionManager.java
jenkins retest this (testing with narayana-jta-5.8.1) |
Closing to merge https://github.com/kiegroup/drools/pull/1939 first. A new PR will replace this one soon. |
Provides integration between jBPM engine, Narayana and Tomcat DBCP. Changes introduced by this PR have been already tested with various DBs and JEE containers.
Depends on https://github.com/kiegroup/drools/pull/1939
[DO NOT MERGE] - waiting for tomcat-dbcp stable release