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

Add a Jakarta JMS Appender #2295 #3247

Merged

Conversation

garydgregory
Copy link
Member

@garydgregory garydgregory commented Nov 28, 2024

Adds a Jakarta JMS Appender #2295

In order to complete an app stack's port from javax to jakarta, I need to be jakarta all the way.

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

I think we should rather proceed as in the Jakarta Mail API case:

  • We create a new log4j-jakarta-jms artifact.
  • We modify JmsAppender to look for JmsManager implementations via ServiceLoader.

Comment on lines 42 to 43
@Plugin(name = "JMS-Jakarta", category = Node.CATEGORY, elementType = Appender.ELEMENT_TYPE, printObject = true)
public class JmsAppender extends AbstractAppender {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to have a single JMS plugin like we have for SMTP:

  • We can have a single JmsAppender and multiple JmsManagers,
  • The JmsAppender.Builder.build() method finds an implementation of JmsManagerFactory using ServiceLoader.
  • If it finds nothing, the default Javax implementation is used.

Copy link
Member Author

Choose a reason for hiding this comment

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

I really don't want to do some service loader magic and finding this or that on the class path. I want to use the old or the new explicitly.

Copy link

github-actions bot commented Nov 28, 2024

Job Requested goals Build Tool Version Build Outcome Build Scan®
build-macos-latest clean install 3.9.8 Build Scan PUBLISHED
build-ubuntu-latest clean install 3.9.8 Build Scan PUBLISHED
build-windows-latest clean install 3.9.8 Build Scan PUBLISHED
Generated by gradle/develocity-actions

@garydgregory garydgregory force-pushed the 2.x_2295_add_JMS_Jakarta_Appender branch 5 times, most recently from 52353a8 to c31ff81 Compare December 1, 2024 14:12
@garydgregory garydgregory force-pushed the 2.x_2295_add_JMS_Jakarta_Appender branch 3 times, most recently from dafbf63 to 504625a Compare December 2, 2024 22:52
@garydgregory
Copy link
Member Author

Hi @ppkarwasz and all: Ready for review.

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

@garydgregory,

Honestly I would prefer for the Jakarta JMS appender to work with the "JMS" name: as far as I understand now users need to change JMS to JMS-Jakarta in their Log4j Core 2 and to change from JMS-Jakarta back to JMS once they migrate to Log4j Core 3?

Another question that we might want to discuss is the reliance on JNDI: the JmsManager actually only needs a JMS ConnectionFactory and it really doesn't matter how this factory is obtained. To configure JMS we could proceed the same way as with JDBC:

  • The JMS-Jakarta plugin could have a nested ConnectionFactoryProvider element to specify how to obtain the ConnectionFactory.
  • We provide an implementation of the ConnectionFactoryProvider implementation that uses JNDI.
  • Third parties (e.g. ActiveMQ Artemis) could provide other ConnectionFactoryProvider. I know that Spring Boot doesn't use JNDI at all with Artemis (see ArtemisConnectionFactoryConfiguration).

What do you think? Of course if there are configuration differences between the Javax and Jakarta JMS appenders, the name change is justified.

log4j-jakarta-jms/pom.xml Outdated Show resolved Hide resolved
log4j-jakarta-jms/pom.xml Outdated Show resolved Hide resolved
Comment on lines +437 to +440
For Jakarta, use the `JMS-Jakarta` element name in the `log4j-jakarta-jms` Maven module.
For Javax, use the `JMS-Javax` element name; the names `JMS`, `JMSQueue`, and `JMSTopic` are provided for backward compatibility.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could use the same technique as with the Servlet Appender: document only the Jakarta version and add a collapsible paragraph "Click here if you are using Jakarta EE 8 or any version of Java EE" (see Servlet Appender documentation).

Copy link
Member Author

Choose a reason for hiding this comment

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

The collapsible UI to too lame to use IMO. It's hard to tell how much is expanded, maybe if it expanded into something with a border it would help. I did not want to click on the "Click here" text because I did not want to leave the page.

The true uselessness of the collapsible thingy is that when you search for text on a page in a web browser (Firefox in my case), the browser does not find the text inside the collapsed text, so using the collapsible thingy an anti-pattern as implemented (IMO). Or maybe Firefox found it but you certainly don't see it.

The new site is hard enough to use without this kind of UI to make it worse 😞... IMO

@garydgregory garydgregory force-pushed the 2.x_2295_add_JMS_Jakarta_Appender branch from 75475c2 to e6392aa Compare December 7, 2024 17:47
@garydgregory
Copy link
Member Author

garydgregory commented Dec 7, 2024

@garydgregory,

Honestly I would prefer for the Jakarta JMS appender to work with the "JMS" name: as far as I understand now users need to change JMS to JMS-Jakarta in their Log4j Core 2 and to change from JMS-Jakarta back to JMS once they migrate to Log4j Core 3?

Hi @ppkarwasz

Ouch? For 2.x, reusing the "JMS" name will force users to migrate from Javax to Jakarta, this is going to be a surprise hidden change and break existing configurations. I don't think we should do that.

The move to 3.x will require plenty of changes for non-trivial configurations IMO, especially when switching from Javax to Jakarta, adding this and that new Log4j modules that are now split out. So I'm OK with keeping the new name, supporting both styles, or renaming.

ATM, I am only concerned about an IRL 2.x use case for JMS and Jakarta. For 3.x we can do whatever.

WDYT?

Another question that we might want to discuss is the reliance on JNDI: the JmsManager actually only needs a JMS ConnectionFactory and it really doesn't matter how this factory is obtained. To configure JMS we could proceed the same way as with JDBC:

* The `JMS-Jakarta` plugin could have a nested `ConnectionFactoryProvider` element to specify how to obtain the `ConnectionFactory`.

* We provide an implementation of the `ConnectionFactoryProvider` implementation that uses JNDI.

* Third parties (e.g. ActiveMQ Artemis) could provide other `ConnectionFactoryProvider`. I know that Spring Boot doesn't use JNDI at all with Artemis (see [`ArtemisConnectionFactoryConfiguration`](https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jms/artemis/ArtemisConnectionFactoryConfiguration.java)).

What do you think? Of course if there are configuration differences between the Javax and Jakarta JMS appenders, the name change is justified.

@garydgregory
Copy link
Member Author

Merging after discussion with @ppkarwasz. Keep it simple and no need for an extra mechanism to detect javax vs jakarta implementations on the classpath.

@garydgregory garydgregory merged commit 43a0e29 into apache:2.x Dec 7, 2024
9 checks passed
@garydgregory garydgregory deleted the 2.x_2295_add_JMS_Jakarta_Appender branch December 7, 2024 21:02
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