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

Feature : Quarkus integration #320

Merged
merged 6 commits into from
Oct 7, 2022

Conversation

RomainWilbert
Copy link
Contributor

I work with Quarkus so I created a module to make transaction-outbox work with cdi-api & transaction-api.

Should work with other implentations.

I have made basic tests, feel free to add some if needed !

@badgerwithagun
Copy link
Member

Thanks @RomainWilbert, and welcome!

I'll give it a quick review now.

@badgerwithagun
Copy link
Member

Added some small thoughts and a couple of questions, but this looks great!

Thank you again for contributing!

@RomainWilbert
Copy link
Contributor Author

Did you take a look at the tests ?

@RomainWilbert
Copy link
Contributor Author

Hi @badgerwithagun !

I am a bit confused regarding my CDI Transaction implementation. I observed that :

  • I inspired myself from the Spring implementation, but I can't figure why the Transaction is created once and for all within the Transaction manager. There should be one transaction for each proxy call ?
  • It seems that the Datasource.getConnection() method returns a different connection on each call...and connections are never closed after proxy calls are done. I reach my connection pool size quite fast (2 new connections for each call).

Thank you for your help

@RomainWilbert
Copy link
Contributor Author

OK i figured out the issue. I had to use CDI instanciation mecanism in order to make the CDITransactionManager aware of any opened transaction. All seems OK and connections are closed at the end of the process

@RomainWilbert RomainWilbert changed the title Feature : CDI integration Feature : Quarkus integration Oct 3, 2022
.github/workflows/release.yml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
transactionoutbox-quarkus/pom.xml Outdated Show resolved Hide resolved
@badgerwithagun badgerwithagun merged commit 121f27d into gruelbox:master Oct 7, 2022
@RomainWilbert RomainWilbert deleted the feat-quarkus-cdi branch November 10, 2022 14:19
@RomainWilbert
Copy link
Contributor Author

RomainWilbert commented Nov 22, 2022

Maybe something is not rightly implemented with the connection because I have warnings in logs
[io.agr.pool] (executor-thread-0) Datasource '<default>': Closing open connection prior to commit

@RomainWilbert
Copy link
Contributor Author

We are benchmarking the module and there are issues with performance and connections lifecycle. This implementation was inspired by the Spring one, but obviously it is not the right angle. I will keep posting updates here

@badgerwithagun
Copy link
Member

Any luck @RomainWilbert ?

@RomainWilbert
Copy link
Contributor Author

False alert. We did some real time debug to ensure everything was OK on that part. Our issues came from elsewhere : a hardcoded timeout on a request in a DAO...
The warning Closing open connection prior to commit comes from the connection wrapper, there is a reaper that checks if connection & statements are closed before commit, and it is called before the afterCompletion event...one way to prevent the warnings is to return the connection instead of the wrapper in the TransactionManager : but required to be careful on statement & connection closing as the reaper secure closing is not called anymore before commit.
public final Connection connection() { try { ConnectionWrapper connectionWrapper = (ConnectionWrapper) datasource.getConnection(); Connection connection = connectionWrapper.getHandler().rawConnection(); log.trace("Connection : " + connection); connectionWrapper.close(); return connection; } catch (SQLException e) { throw new RuntimeException(e); } }

badgerwithagun pushed a commit that referenced this pull request May 14, 2023
* feat: cdi implementation

* fix: PR comments

* fix: cdi transaction manager instantiation to be transaction aware

* Update CdiTransactionManager.java

* ref: module name (quarkus)

* ref: module name

Co-authored-by: Romain Wilbert <romain.wilbert-ext@pole-emploi.fr>

(cherry picked from commit 121f27d)
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.

2 participants