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

Upgrade to Narayana 6.0.0.CR1 in the Jakarta branch #30475

Closed
gsmet opened this issue Jan 19, 2023 · 12 comments · Fixed by #30746
Closed

Upgrade to Narayana 6.0.0.CR1 in the Jakarta branch #30475

gsmet opened this issue Jan 19, 2023 · 12 comments · Fixed by #30746
Assignees
Labels
area/jakarta area/narayana Transactions / Narayana
Milestone

Comments

@gsmet
Copy link
Member

gsmet commented Jan 19, 2023

My guess is that we will have to tweak the rewrite rules to go back to the standard artifacts which should be Jakarta-enabled now.

@gsmet gsmet self-assigned this Jan 19, 2023
@gsmet gsmet added area/narayana Transactions / Narayana area/jakarta and removed triage/needs-triage labels Jan 19, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Jan 19, 2023

/cc @manovotn (jakarta), @maxandersen (jakarta), @mmusgrov (narayana), @radcortez (jakarta), @Sanne (jakarta)

@Sanne
Copy link
Member

Sanne commented Jan 19, 2023

these coordinate changes seemed to work fine in Hibernate ORM:

@mmusgrov any other transitive dependencies we should be mindful of?

@Sanne
Copy link
Member

Sanne commented Jan 19, 2023

Actually, that previous commit failed non-core integration tests; this should work:

@gsmet gsmet added triage/needs-triage area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins and removed triage/needs-triage labels Jan 19, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Jan 19, 2023

/cc @manovotn (jakarta), @maxandersen (jakarta), @mmusgrov (narayana), @radcortez (jakarta), @Sanne (jakarta)

@gsmet gsmet removed the area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins label Jan 19, 2023
@mmusgrov
Copy link
Contributor

@mmusgrov any other transitive dependencies we should be mindful of?

@Sanne The JTA spec API only specifies two dependencies: jakarta.enterprise.cdi-api and jakarta.interceptor-api (https://github.com/jakartaee/transactions/blob/master/api/pom.xml#L315). Narayana itself pulls in lots of jakarta dependencies (cd ArjunaJTA; mvn dependency:tree | grep jakarta).

@Sanne
Copy link
Member

Sanne commented Jan 19, 2023

thanks @mmusgrov , but yes I was asking about the Narayana dependencies: we can ignore the build time and test dependencies, but for each of the runtime dependencies we will need them aligned to the BOM in Quarkus - especially in case other components might depend on the same.

Sometimes there is more flexibility, other times you might need a very specific version, so it would be great if the owning team could provide some guidance beyond the content of the pom files.

Incidentally, the pom files seem incomplete as well as I had to manually pull in the CDI and Jakarta Interceptor APIs; if these are required I would expect them to be declared as transitive dependencies of Narayana.

@mmusgrov
Copy link
Contributor

@Sanne I made a local fix and built 6.0.0.Final-SNAPSHOT. But when I include this version in hibernate-orm like this

alias( "jbossJta" ).to( "org.jboss.narayana.jta", "narayana-jta" ).version( "6.0.0.Final-SNAPSHOT" )

the build fails with Could not find org.jboss.narayana.jta:narayana-jta:6.0.0.Final-SNAPSHOT.

How do I tell grade to look in my local repo when resolving 6.0.0.Final-SNAPSHOT?

@mmusgrov
Copy link
Contributor

Found it (I needed to include mavenLocal() in the dependencyResolutionManagement section of settings.gradle). I'll update 6.0.0.Final-SNAPSHOT with the missing transitive dependencies so the fix will appear when we release 6.0.0.Final.

Regarding your other comment about aligning Narayana dependencies with versions as specified in the Quarkus BOM. Are you referring to versions of Jakarta dependencies that Narayana and Quarkus share or more than just those.

@Sanne
Copy link
Member

Sanne commented Jan 23, 2023

Found it (I needed to include mavenLocal() in the dependencyResolutionManagement section of settings.gradle). I'll update 6.0.0.Final-SNAPSHOT with the missing transitive dependencies so the fix will appear when we release 6.0.0.Final.

Right, exactly that. Thanks!

Regarding your other comment about aligning Narayana dependencies with versions as specified in the Quarkus BOM. Are you referring to versions of Jakarta dependencies that Narayana and Quarkus share or more than just those.

I'm referring to any dependency that Naryana will require: including Jakarta APIs but not only. If there's anything in the Quarkus BOM that you know needs to be strictly aligned with Narayana needs.

In general we can rely on integration tests, but since integration testing doesn't have as high coverage as Narayana's own testsuite, it might be best for you to occasionally have a look - especially if you know of strict expectations, such as cases in which you had to upgrade a dependency to a particular version to fix/avoid some issues.

Sometimes the opposite is true as well; for example I just upgraded Hibernate ORM's dependencies to match some newer versions of dependencies that Quarkus is using.
For example:

Then when all dependencies are aligned we have the reassurance of both integration testsuites; or in this case we have the benefit of all three integration testsuites converging, since I upgraded Hibernate ORM to Narayana 6.0.0.CR1 as well ;)

@mmusgrov
Copy link
Contributor

mmusgrov commented Jan 30, 2023

Thanks @Sanne we have started working on a narayana bom (narayana pr/2079) which will help us with aligning dependencies.

Regarding the transitive dependency issue; we already include jakarta.enterprise.cdi-api in the narayana-jta pom [1].

I also created a small JTA transaction example [2] whose pom only pulls in org.jboss.narayana.jta:narayana-jta:6.0.0.CR1 (and org.jboss.logging:jboss-logging:3.5.0.Final) - it runs without issue. How can I test your transitive dependency issue other than running/debugging your hibernate-orm tests?

[1] https://github.com/jbosstm/narayana/blob/main/ArjunaJTA/narayana-jta/pom.xml#L135
[2]

        TransactionExample txeg = new TransactionExample();
        
        txeg.commitUserTransaction();
        txeg.commitTransactionManager();
        txeg.rollbackUserTransaction();
        txeg.setRollbackOnly();
        txeg.transactionStatus();
        txeg.transactionTimeout();

@Sanne
Copy link
Member

Sanne commented Jan 30, 2023

thanks @mmusgrov , the failures of Hibernate ORM's integration tests are triggered when we run it on the modules path; but let's not digress too much about that here as this is the Quarkus PR - I was just letting you know, as Narayana end users might benefit from those dependencies being included, but it's not essential to Quarkus.

Thanks for the link to the pom file - I'll use that to check alignment with Quarkus and send subsequent PRs as (and if) needed.

@Sanne
Copy link
Member

Sanne commented Jan 30, 2023

@mmusgrov I found a single dependency misaligned - not sure how important it is:

Narayana has a dependency to io.smallrye.reactive:smallrye-reactive-converter-api and seems to be using version 1.0.10; in Quarkus we're using version 2.7.0 - that seems like a significant difference, I'd recommend to upgrade Narayana to make sure it's not a problem?

gsmet added a commit that referenced this issue Jan 31, 2023
gsmet added a commit to gsmet/quarkus that referenced this issue Jan 31, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Quarkus Roadmap/Planning Feb 1, 2023
@quarkus-bot quarkus-bot bot added this to the 2.17 - main milestone Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/jakarta area/narayana Transactions / Narayana
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants