Fix XA support for Oracle in native #40704
Conversation
quarkus-bot
bot
added
area/docstyle
|
/cc @yrodiere (hibernate-orm) |
|
@zakkak I wonder if we should also register nested classes by default with the build item (and provide a way to disable it) as it's a bit weird the build item and the annotation are not consistent. Especially since, in some cases, it's very inconvenient to register all the nested classes manually. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🙈 The PR is closed and the preview is expired. |
As someone who doesn't have to deal with this often enough I think I am not the right person to make that call. I am certainly OK with such a change if it makes things easier for Quarkus contributors and users. |
| @@ -3,6 +3,7 @@ quarkus.datasource.username=SYSTEM | |||
| quarkus.datasource.password=hibernate_orm_test | |||
| quarkus.datasource.jdbc.url=${oracledb.url} | |||
| quarkus.datasource.jdbc.max-size=1 | |||
| quarkus.datasource.jdbc.transactions=xa | |||
There was a problem hiding this comment.
This is nice, but... it means we're no longer testing the non-XA case with Oracle.
Should we duplicate all such test modules? :/
There was a problem hiding this comment.
I thought the second datasource would be fine? But maybe it doesn't connect actually.
I can drop this part or add have 2 datasources that point to the same database but one xa and one non-xa?
There was a problem hiding this comment.
Given we will push more people towards XA, I think it's probably better to have separate tests for XA for each driver we support. It has a cost but I don't feel comfortable not paying it given the new situation with Agroal/Narayana.
WDYT?
There was a problem hiding this comment.
My concerns are mostly about native images, where the simple fact of using of XA could have a dramatic impact on what GraalVM generates.
I agree duplicating tests would be the way forward.
This will mean even more copy-pasting in tests that already contain a lot of that, but until we find a way to run the same integration test multiple times with different configuration and Maven dependencies... that will have to do.
There was a problem hiding this comment.
I dropped the test change for now.
Will see if it makes sense to have so many new native modules later.
We need to register a few more classes for reflection and also all their nested classes, which is why I use `@RegisterForReflection` instead of the usual build item. Fixes quarkusio#23341
Status for workflow
|
Status for workflow
|
We need to register a few more classes for reflection and also all their
nested classes, which is why I use
@RegisterForReflectioninstead ofthe usual build item.
Fixes #23341