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

Adapt to SpringBoot 3, Camel 4 and Jakarta EE, removing DBCP feature and deprecated methods #99

Merged
merged 2 commits into from
Jun 27, 2023

Conversation

graben
Copy link
Contributor

@graben graben commented May 20, 2023

Major upgrade to latest Spring-Boot, Narayana and several other dependencies, replacing incompatible DBCP feature with Agroal.

Resolves #95
Resolves #97
Depends: agroal/agroal#63, agroal/agroal#65, agroal/agroal#66, agroal/agroal#67, agroal/agroal#68

@zhfeng
Copy link
Contributor

zhfeng commented May 22, 2023

Thanks @graben and how io.agroal:agroal-spring-boot-starter can work with narayana ? I didn't see any test with agroal-spring-boot.

@graben
Copy link
Contributor Author

graben commented May 22, 2023

Hi @zhfeng, io.agroal:agroal-spring-boot-starter comes with Narayana integration out of the box, nothing to do. I'll add tests once a few bugs are solved at agroal.

@zhfeng
Copy link
Contributor

zhfeng commented May 22, 2023

OK - if that is the case, I think we should disable XADataSourceWrapper when using io.agroal:agroal-spring-boot-starter. agroal could take care all of the transaction integration.

Please make sure the transaction crash recovery is still working when you are adding test with agroal.

@graben
Copy link
Contributor Author

graben commented May 22, 2023

Well, XADataSourceWrapper is still useful for non pooled DataSources which are not handled by agroal. Pooling has been completely removed from sourcecode, yet. The code at agroal has been developed by some Narayana guys from RedHat, I think they know what they are doing. ;-)

@zhfeng
Copy link
Contributor

zhfeng commented May 22, 2023

When using with agroal-spring-boot, XADataSourceWrapper will still wrap the DatsSource creaing by agroal-spring-boot?

@zhfeng
Copy link
Contributor

zhfeng commented May 22, 2023

@graben Can I ask for taking the removing dbcp in a seperate commit ? Since there is a discussion to support jakarta in dbcp, so it could be easy to bring dbcp back in the future.

Thanks a lot!

@graben
Copy link
Contributor Author

graben commented May 22, 2023

@zhfeng , I can restore DBCP support at any time. IMHO the discussion at commons is never ending, that's why I took the step forward for an alternative way. Btw agroal is creating a DataSource instance, so no problem with rewrapping by XADataSourceWrapper and they use narayana-spring-boot for their tests.

@zhfeng
Copy link
Contributor

zhfeng commented May 22, 2023

@graben - I see.

For rewrapping, the only thing I concern is about registering the XAResourceRecoveryHelper. It seems that if we don't disable XADataSourceWrapper, there could be two XAResourceRecoveryHelper (one from agroal, and the other from GenericXADataSourceWrapper then during crash recovering, the recovery manager could confuse the same Xids from the different XAResourceRecovery. That might be a potenial issue.

@graben
Copy link
Contributor Author

graben commented May 22, 2023

Since no XADataSource instance is created, there is no problem with the GenericXADataSourceWrapper. Only one XAResourceRecoveryHelper is registered. But I can double-check with debugger in my personnel prototype app.

@zhfeng
Copy link
Contributor

zhfeng commented May 22, 2023

Hmm, agroal will only create DataSource but not XADataSource? It should be OK. Anyway, thank for double-checking. I might be too worries.

@graben graben force-pushed the sb3 branch 3 times, most recently from 61b5a05 to 7ba7ed5 Compare May 25, 2023 07:53
@graben
Copy link
Contributor Author

graben commented May 25, 2023

Hi @zhfeng, I split the commit into two parts as requested.

@zhfeng
Copy link
Contributor

zhfeng commented May 25, 2023

Thanks @graben - LGTM!

@graben graben force-pushed the sb3 branch 2 times, most recently from 202aa7f to 0e75dc7 Compare May 27, 2023 06:47
@graben graben marked this pull request as ready for review May 27, 2023 06:53
@graben graben force-pushed the sb3 branch 2 times, most recently from dafa5bd to 71cc12a Compare June 3, 2023 07:21
Croway referenced this pull request in jboss-fuse/narayana-spring-boot Jun 5, 2023
@graben
Copy link
Contributor Author

graben commented Jun 6, 2023

@zhfeng: Changes good to be merged? It seems that your Fuse colleges at RedHat are fine with the changes proposed!

@zhfeng
Copy link
Contributor

zhfeng commented Jun 7, 2023

Thanks @graben - It looks good to me!

@graben
Copy link
Contributor Author

graben commented Jun 11, 2023

@jacobdotcosta : Could you pls review and merge?

@jacobdotcosta
Copy link
Member

I don't think I'm the right person to review this PR, could you take a look at it @Sgitario , please?

@Sgitario
Copy link
Contributor

I don't think I'm the right person to review this PR, could you take a look at it @Sgitario , please?

I have never worked in this repository either @jacobdotcosta

@geoand
Copy link
Member

geoand commented Jun 27, 2023

It looks fine from a strictly Spring Boot perspective

@graben
Copy link
Contributor Author

graben commented Jun 27, 2023

@geoand : Would you like to approve and merge? The number of repository owners is quite small! Same for my other PRs.

@geoand geoand merged commit 67f4a49 into snowdrop:main Jun 27, 2023
@graben graben deleted the sb3 branch June 27, 2023 17:43
@johnpoth
Copy link

Is there a release planned soon containing these changes ? Thanks !

@geoand
Copy link
Member

geoand commented Jul 17, 2023

@jacobdotcosta ^

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.

Spring Boot 3.0 support Support for Spring Boot 3
6 participants