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

[WFLY-15548] [WFLY-15446] transaction subsystem: jakarta namespance p… #14975

Closed
wants to merge 3 commits into from

Conversation

mmusgrov
Copy link
Contributor

@mmusgrov mmusgrov commented Nov 29, 2021

https://issues.redhat.com/browse/WFLY-15548 (the transaction subsystem module)
https://issues.redhat.com/browse/WFLY-15446 (I think this issue is superfluous and I've asked on the issue for clarification)

WildFly Preview for the Jakarta namespace variant of the transaction subsystem module.

Remark: this PR also upgrades the narayana component for which I have raised a separate #14974

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Nov 29, 2021
<version>26.0.0.Beta1-SNAPSHOT</version>
</parent>

<artifactId>wildfly-transactions-jakarta</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @mmusgrov , if this is not a work in progress PR, essentially, this is just adding a new maven module. You also need to use it for the wildfly-preview feature pack.

From the Brian's tutorial, I'm missing the steps from 9) to 13) for this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the link, I will update the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yersan the latest version of narayana (5.12.4.Final), which is included in this PR, has introduced a performance regression of the transaction log store. I've been trying to track it down this week (the perf jobs takes the best part of a day to run). If we go ahead with this PR then we will still need another narayana release before 26.0.0 can be released.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yersan Just an FYI, we plan to merge the narayana component upgrade with the regression and follow up asap once the dependent library has been fixed (the current plan is sometime next week for the dependency to be fixed).

And I updated my branch to add the missing steps from 9) to 13) but the build fails on the wildfly-preview-feature-pack module and I don't know how to debug that so I asked the question in the wildfly forums (https://lists.jboss.org/archives/list/wildfly-dev@lists.jboss.org/thread/ZCXWAOVBRTOI247RZQ4X5UUVEITI3ZQJ/). Is that the best place to ask that question?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mmusgrov , I reply to your question in the WildFly dev List. If you still have trouble building it, feel free to upgrade the PR with what you have and we can discuss the details here on GitHub.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yersan for the debug tip which shows the reason for the failure:

Caused by: java.lang.IllegalStateException: WFLYCTL0153: No META-INF/services/org.jboss.as.controller.Extension found for org.jboss.as.transactions

We have one of those in the subsystem code:

<WFLY repo>/transactions/src/main/resources/META-INF/services/org.jboss.as.controller.Extension

Do we also need one somewhere in the ee-9 part of the source tree?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmusgrov No really, none of the other subsystems already transformed do anything related to these kinds of services.

I'm not sure about your current structure, can you push your latest version of this PR so we can see what you have at the moment and suggest what is still missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yersan I have pushed my updates for steps 9) to 13)

Comment on lines 74 to 79
<version>5.12.4.Final</version>
</dependency>
<dependency>
<groupId>org.jboss.narayana.jts</groupId>
<artifactId>narayana-jts-integration-jakarta</artifactId>
<version>5.12.4.Final</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These modules should be inherited from the main ee-9/pom.xml

@darranl darranl added the hold PR should not be merged for some reason. label Dec 6, 2021
@darranl
Copy link
Contributor

darranl commented Dec 6, 2021

Just adding hold as requested by @mmusgrov

@darranl darranl added the rebase-this PR has a merge conflict. label Jan 10, 2022
@mmusgrov mmusgrov force-pushed the WFLY-15548-and-WFLY-15446 branch 4 times, most recently from d08b5bd to 7953213 Compare January 24, 2022 12:42
Comment on lines 759 to 765
<exclusion>
<groupId>${ee.maven.groupId}</groupId>
<artifactId>wildfly-transactions-jakarta</artifactId>
</exclusion>
Copy link
Contributor

@yersan yersan Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea behind this is to avoid introducing in the build classpath the Jakarta EE 8 extension. The exclusion here should be:

<exclusion>
         <groupId>${ee.maven.groupId}</groupId>
         <artifactId>wildfly-transactions</artifactId>
</exclusion>

@mmusgrov That's probably the root of your issue with the missing service.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yersan I pushed the update but I still get the same issue after making that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed two of your comments, I am retesting after addressing those.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yersan But I still get the error. Do you still have your branch where you managed to get it working, if so I can clone that and use it to compare with my updates after addressing your comments?

Copy link
Contributor

@yersan yersan Jan 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmusgrov The build succeeds if you place the missing dependencies before wildfly-iiop-openjdk, which indeed is wrong, we should use wildfly-iiop-openjdk-jarkarta.

Having said that, there is still additional work to do:

  • Use wildfly-iiop-openjdk-jarkarta instead of wildfly-iiop-openjdk
  • Exclude whatever is not needed from the wildfly-ee-feature-pack-common, e.g narayana-jts-integration,narayana-jts-idlj and wildfly-iiop-openjdk. I've noticed they are still being transformed, but they shouldn't.
  • Avoid the build being dependent on the dependency order declaration, it should work if you place the dependencies at the beginning or at the end.

Let me know if you want me to take a closer look at all of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yersan
Which missing dependencies?
Which dependencies depend on the order of declaration?

And the build still fails with the same error WFLYCTL0153: No META-INF/services/org.jboss.as.controller.Extension found for org.jboss.as.transactions after addressing your first two bullets. These extra requirements are not mentioned in Brian's document so I am just guessing at these extra items you say I need.

I would take you up on your offer of "taking a closer look at all of this" but, since I am just guessing at these extra items I will have the same problems with the other systems I have pending updates for, namely wildfly-rts-jakarta and wildfly-xts-jakarta, and I suspect your good will won't extend that far.

<name>WildFly: Transaction Subsystem (Jakarta Namespace)</name>

<properties>
<transformer-input-dir>${project.basedir}/../../../../transactions</transformer-input-dir>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found that this module is three levels above the transactions module, instead of four. So, replace it by:

<transformer-input-dir>${project.basedir}/../../../transactions</transformer-input-dir>

Comment on lines 46 to 119
<dependencies>
<dependency>
<groupId>org.wildfly.core</groupId>
<artifactId>wildfly-controller</artifactId>
</dependency>
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>wildfly-ee-jakarta</artifactId>
</dependency>
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>wildfly-iiop-openjdk</artifactId>
</dependency>
<dependency>
<groupId>org.jboss.openjdk-orb</groupId>
<artifactId>openjdk-orb</artifactId>
</dependency>
<dependency>
<groupId>org.jboss</groupId>
<artifactId>jboss-transaction-spi-jakarta</artifactId>
</dependency>
<dependency>
<groupId>org.jboss.remoting</groupId>
<artifactId>jboss-remoting</artifactId>
</dependency>
<dependency>
<groupId>org.jboss.logging</groupId>
<artifactId>jboss-logging-annotations</artifactId>
<!-- This is a compile-time dependency of this project, but is not needed at compile or runtime by other
projects that depend on this project.-->
<scope>provided</scope>
<optional>true</optional>
</dependency>
<dependency>
<groupId>org.jboss.logging</groupId>
<artifactId>jboss-logging-processor</artifactId>
<!-- This is a compile-time dependency of this project, but is not needed at compile or runtime by other
projects that depend on this project.-->
<scope>provided</scope>
<optional>true</optional>
</dependency>
<dependency>
<groupId>jakarta.resource</groupId>
<artifactId>jakarta.resource-api</artifactId>
</dependency>
<dependency>
<groupId>jakarta.transaction</groupId>
<artifactId>jakarta.transaction-api</artifactId>
</dependency>
<dependency>
<groupId>org.wildfly.transaction</groupId>
<artifactId>wildfly-transaction-client-jakarta</artifactId>
</dependency>

<dependency>
<groupId>org.wildfly.wildfly-http-client</groupId>
<artifactId>wildfly-http-transaction-client</artifactId>
</dependency>

<dependency>
<groupId>org.wildfly.common</groupId>
<artifactId>wildfly-common</artifactId>
</dependency>
<dependency>
<groupId>org.wildfly.core</groupId>
<artifactId>wildfly-subsystem-test</artifactId>
<type>pom</type>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.wildfly.security</groupId>
<artifactId>wildfly-elytron-security-manager</artifactId>
</dependency>
</dependencies>
Copy link
Contributor

@yersan yersan Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also found some missing dependencies:

<dependency>
            <groupId>jakarta.enterprise.concurrent</groupId>
            <artifactId>jakarta.enterprise.concurrent-api</artifactId>
        </dependency>
        <dependency>
            <groupId>org.glassfish</groupId>
            <artifactId>jakarta.enterprise.concurrent</artifactId>
        </dependency>
        <dependency>
            <groupId>org.jboss.narayana.jts</groupId>
            <artifactId>narayana-jts-integration-jakarta</artifactId>
        </dependency>
        <dependency>
            <groupId>org.jboss.narayana.jts</groupId>
            <artifactId>narayana-jts-idlj-jakarta</artifactId>
        </dependency>

narayana-jts-integration-jakarta must be also added on the ee-9/pom.xml.

After adding them I'm able to compile and run the tests in that module.

As a final note, it would be desirable to keep these dependencies grouped by those which are Jakarta's native namespace and those that aren't. We have been doing the same for the other transformed modules. In general, it helps to see if we have something pending to upgrade on that module.

@darranl darranl removed the rebase-this PR has a merge conflict. label Jan 26, 2022
@yersan
Copy link
Contributor

yersan commented Feb 2, 2022

Superseded by #15171

This can be closed now.

@mmusgrov mmusgrov closed this Feb 3, 2022
@mmusgrov
Copy link
Contributor Author

mmusgrov commented Feb 3, 2022

Superseded by #15171

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes hold PR should not be merged for some reason.
Projects
None yet
4 participants