You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@logging.apache.org by "Piotr P. Karwasz" <pi...@gmail.com> on 2022/03/05 08:31:10 UTC
Jakarta based appenders
Hello,
A month ago Andy Wilkinson (from Spring Boot I presume) raised the
issue of the lack of support in Log4j 2.x for the Jakarta version of
Java Mail (LOG4J2-3362).
In Log4j 3.x adding such a feature is trivial (copy 'log4j-smtp`
artifact, run Tomcat's conversion tool and update the dependencies),
but I am not sure if Log4j 3.x will be released in time for Spring
Boot 3.x (which has a baseline of Jakarta EE 9).
In Log4j 2.x both the SMTP and JMS appenders are in `log4j-core`,
which complicates the situation. A month ago I submitted:
https://github.com/apache/logging-log4j2/pull/745
which was unacceptable, because it added the Jakarta SMTP manager
directly to `log4j-core` and added new dependencies.
Thanks to your objections I refurbished it with these assumptions:
* the Java EE SMTP appender and manager stay in `log4j-core`,
* a new 'log4j-jakarta-smtp` artifact is introduced, which only
contains the manager part of the Jakarta EE SMTP appender, so that
users can still use the "SMTP" plugin name in their configuration,
* everything is glued together using reflection: if the SMTP appender
(in `log4j-core`) detects the presence of `log4j-jakarta-smtp` it
switches to the Jakarta manager. There is of course also a manual
on/off switch.
Can someone have a look at the PR, whether it is acceptable?
With Java Mail there is also the problem of which dependencies to
declare: there is a separate artifact for the Java Mail API, but the
`com.sun.mail` implementation artifacts include the API classes. There
are no API-less artifacts as in the case of JAXB (the `org.glassfish`
artifacts). Therefore I reconfigured the dependencies of `log4j-core`
to include:
* the Java Activation and Mail APIs as optional compile dependencies
(should it be 'provided'?),
* the implementation 'java.mail' artifact as runtime dependency.
Does it seem Ok? We could also create a separate `log4j-smtp` artifact
of type POM, which would pull those dependencies without the
"optional" part. Just to prepare users for Log4j 3.x.
Piotr
Re: Jakarta based appenders
Posted by Matt Sicker <bo...@gmail.com>.
That ServiceLoaderUtil idea could be relevant to my open DI PR where I refactored some related things to that.
—
Matt Sicker
> On Mar 6, 2022, at 04:10, Piotr P. Karwasz <pi...@gmail.com> wrote:
>
> Hi Ralph,
>
>> On Sat, Mar 5, 2022 at 6:16 PM Ralph Goers <ra...@dslextreme.com> wrote:
>> In core, so long as the SmtpAppender has to remain in core then it needs to
>> determine if the Jakarta support is present or not and use it if it is. But I would
>> suggest it do that using ServiceLoader instead of reflection. If the Jakarta version
>> is present it should be given preference over the default version.
>
> That is a very nice idea. I'll rewrite the PR to use a ServiceLoader,
> so that the manager factory will be chosen by the first match in the
> list:
> 1. The value of the Log4j system property
> "org.apache.logging.log4j.core.net.MailManagerFactory" (an interface
> that all SMTP manager factories will implement),
> 2. A service implementing the same interface,
> 3. The default "org.apache.logging.log4j.core.net.SmtpManager.SMTPManagerFactory".
>
> A nice thing to have would be an equivalent of JAXB's `ServiceLoaderUtil`:
> https://github.com/eclipse-ee4j/jaxb-api/blob/master/api/src/main/java/jakarta/xml/bind/ServiceLoaderUtil.java
> (actually this is copy/pasted in many Java EE artifacts). This way we
> can also deal with the OSGI case by delegating to the
> `osgi-resource-locator`.
>
> The `ServiceLoader` mechanism can actually be applied to other non
> Java EE appenders: e.g. in the current version the `HttpAppender` is
> hardcoded to use a `HttpManager` based on the `HttpURLConnection`.
>
> Piotr
Re: Jakarta based appenders
Posted by Ralph Goers <ra...@dslextreme.com>.
You might want to look at the one in master. It probably needs the same thing done.
However, it also uses modules which is quite a bit simpler since we don’t have to
search for ClassLoaders.
If you look at ProviderUtil you will notice that the Provider interface specifies a “version”
of the API. If the service doesn’t match that version it doesn’t get loaded.
Although this isn’t an exact match it should illustrate how to implement what you want:
1. Define an interface for the service. It should include something like int getPriority();
2. In the service implementation have the Priority value be hard-coded to a value.
When you call ServiceLoader call the getPriority method and rank the located services
in priority order.
Ralph
> On Mar 6, 2022, at 11:56 AM, Piotr P. Karwasz <pi...@gmail.com> wrote:
>
> On Sun, Mar 6, 2022 at 6:23 PM Ralph Goers <ra...@dslextreme.com> wrote:
>> I wrote a ServiceLoaderUti. It is in log4j-api in master. However, it has shortcomings.
>> ServliceLoader MUST be invoked in the appropriate JPMS module or it won’t find
>> the correct declarations in module-info.java. So ServiceLoaderUtil requires a lambda
>> where the ServiceLoader call must be performed.
>
> I might have sworn I looked for all references to `ServiceLoader` in
> `master` too and I didn't find this one. :-(
>
> Anyway I sent what I need in `release-2.x` as:
>
> https://github.com/apache/logging-log4j2/pull/787
>
> where I concentrated on two shortcomings of a simple `ServiceLoader#load`:
>
> 1. Resources found in the parent classloader of a web application
> classloader can be useless (they extend a class in the parent
> classloader, not the class requested),
> 2. The order in which services are found is random, so for
> applications that require a single service, we need a way to override
> the choice provided by `ServiceLoader`.
>
> I'll refactor the PR to use the same method names and expand the
> `master` version to provide the two features above.
>
> Piotr
Re: Jakarta based appenders
Posted by "Piotr P. Karwasz" <pi...@gmail.com>.
On Sun, Mar 6, 2022 at 6:23 PM Ralph Goers <ra...@dslextreme.com> wrote:
> I wrote a ServiceLoaderUti. It is in log4j-api in master. However, it has shortcomings.
> ServliceLoader MUST be invoked in the appropriate JPMS module or it won’t find
> the correct declarations in module-info.java. So ServiceLoaderUtil requires a lambda
> where the ServiceLoader call must be performed.
I might have sworn I looked for all references to `ServiceLoader` in
`master` too and I didn't find this one. :-(
Anyway I sent what I need in `release-2.x` as:
https://github.com/apache/logging-log4j2/pull/787
where I concentrated on two shortcomings of a simple `ServiceLoader#load`:
1. Resources found in the parent classloader of a web application
classloader can be useless (they extend a class in the parent
classloader, not the class requested),
2. The order in which services are found is random, so for
applications that require a single service, we need a way to override
the choice provided by `ServiceLoader`.
I'll refactor the PR to use the same method names and expand the
`master` version to provide the two features above.
Piotr
Re: Jakarta based appenders
Posted by Ralph Goers <ra...@dslextreme.com>.
I wrote a ServiceLoaderUti. It is in log4j-api in master. However, it has shortcomings.
ServliceLoader MUST be invoked in the appropriate JPMS module or it won’t find
the correct declarations in module-info.java. So ServiceLoaderUtil requires a lambda
where the ServiceLoader call must be performed.
Ralph
> On Mar 6, 2022, at 3:10 AM, Piotr P. Karwasz <pi...@gmail.com> wrote:
>
> Hi Ralph,
>
> On Sat, Mar 5, 2022 at 6:16 PM Ralph Goers <ra...@dslextreme.com> wrote:
>> In core, so long as the SmtpAppender has to remain in core then it needs to
>> determine if the Jakarta support is present or not and use it if it is. But I would
>> suggest it do that using ServiceLoader instead of reflection. If the Jakarta version
>> is present it should be given preference over the default version.
>
> That is a very nice idea. I'll rewrite the PR to use a ServiceLoader,
> so that the manager factory will be chosen by the first match in the
> list:
> 1. The value of the Log4j system property
> "org.apache.logging.log4j.core.net.MailManagerFactory" (an interface
> that all SMTP manager factories will implement),
> 2. A service implementing the same interface,
> 3. The default "org.apache.logging.log4j.core.net.SmtpManager.SMTPManagerFactory".
>
> A nice thing to have would be an equivalent of JAXB's `ServiceLoaderUtil`:
> https://github.com/eclipse-ee4j/jaxb-api/blob/master/api/src/main/java/jakarta/xml/bind/ServiceLoaderUtil.java
> (actually this is copy/pasted in many Java EE artifacts). This way we
> can also deal with the OSGI case by delegating to the
> `osgi-resource-locator`.
>
> The `ServiceLoader` mechanism can actually be applied to other non
> Java EE appenders: e.g. in the current version the `HttpAppender` is
> hardcoded to use a `HttpManager` based on the `HttpURLConnection`.
>
> Piotr
Re: Jakarta based appenders
Posted by "Piotr P. Karwasz" <pi...@gmail.com>.
Hi Ralph,
On Sat, Mar 5, 2022 at 6:16 PM Ralph Goers <ra...@dslextreme.com> wrote:
> In core, so long as the SmtpAppender has to remain in core then it needs to
> determine if the Jakarta support is present or not and use it if it is. But I would
> suggest it do that using ServiceLoader instead of reflection. If the Jakarta version
> is present it should be given preference over the default version.
That is a very nice idea. I'll rewrite the PR to use a ServiceLoader,
so that the manager factory will be chosen by the first match in the
list:
1. The value of the Log4j system property
"org.apache.logging.log4j.core.net.MailManagerFactory" (an interface
that all SMTP manager factories will implement),
2. A service implementing the same interface,
3. The default "org.apache.logging.log4j.core.net.SmtpManager.SMTPManagerFactory".
A nice thing to have would be an equivalent of JAXB's `ServiceLoaderUtil`:
https://github.com/eclipse-ee4j/jaxb-api/blob/master/api/src/main/java/jakarta/xml/bind/ServiceLoaderUtil.java
(actually this is copy/pasted in many Java EE artifacts). This way we
can also deal with the OSGI case by delegating to the
`osgi-resource-locator`.
The `ServiceLoader` mechanism can actually be applied to other non
Java EE appenders: e.g. in the current version the `HttpAppender` is
hardcoded to use a `HttpManager` based on the `HttpURLConnection`.
Piotr
Re: Jakarta based appenders
Posted by Ralph Goers <ra...@dslextreme.com>.
I have to disagree with Gary. As I recall, in 3.0 you pick which flavor you want as
a dependency and both expose the same plugin. Obviously then you could not
have both on the classpath but that would be expected.
In core, so long as the SmtpAppender has to remain in core then it needs to
determine if the Jakarta support is present or not and use it if it is. But I would
suggest it do that using ServiceLoader instead of reflection. If the Jakarta version
is present it should be given preference over the default version.
Ralph
> On Mar 5, 2022, at 4:57 AM, Gary Gregory <ga...@gmail.com> wrote:
>
> IMO we should not do any reflection magic, you just say which module you
> want, nice and simple. Internally, this might mean we alias the appender
> names just like we did for mongodb Appenders when MongoDb 4 came out. I'm
> not sure what the names should be in this case. Prefixing Appender names
> with Jakarata or JEE/Jee is fine w me but unlikely to please others, as
> would renaming the old Appender to something else.
>
> Gary
>
> Gary
>
> On Sat, Mar 5, 2022, 03:31 Piotr P. Karwasz <pi...@gmail.com> wrote:
>
>> Hello,
>>
>> A month ago Andy Wilkinson (from Spring Boot I presume) raised the
>> issue of the lack of support in Log4j 2.x for the Jakarta version of
>> Java Mail (LOG4J2-3362).
>>
>> In Log4j 3.x adding such a feature is trivial (copy 'log4j-smtp`
>> artifact, run Tomcat's conversion tool and update the dependencies),
>> but I am not sure if Log4j 3.x will be released in time for Spring
>> Boot 3.x (which has a baseline of Jakarta EE 9).
>>
>> In Log4j 2.x both the SMTP and JMS appenders are in `log4j-core`,
>> which complicates the situation. A month ago I submitted:
>>
>> https://github.com/apache/logging-log4j2/pull/745
>>
>> which was unacceptable, because it added the Jakarta SMTP manager
>> directly to `log4j-core` and added new dependencies.
>>
>> Thanks to your objections I refurbished it with these assumptions:
>>
>> * the Java EE SMTP appender and manager stay in `log4j-core`,
>> * a new 'log4j-jakarta-smtp` artifact is introduced, which only
>> contains the manager part of the Jakarta EE SMTP appender, so that
>> users can still use the "SMTP" plugin name in their configuration,
>> * everything is glued together using reflection: if the SMTP appender
>> (in `log4j-core`) detects the presence of `log4j-jakarta-smtp` it
>> switches to the Jakarta manager. There is of course also a manual
>> on/off switch.
>>
>> Can someone have a look at the PR, whether it is acceptable?
>>
>> With Java Mail there is also the problem of which dependencies to
>> declare: there is a separate artifact for the Java Mail API, but the
>> `com.sun.mail` implementation artifacts include the API classes. There
>> are no API-less artifacts as in the case of JAXB (the `org.glassfish`
>> artifacts). Therefore I reconfigured the dependencies of `log4j-core`
>> to include:
>>
>> * the Java Activation and Mail APIs as optional compile dependencies
>> (should it be 'provided'?),
>> * the implementation 'java.mail' artifact as runtime dependency.
>>
>> Does it seem Ok? We could also create a separate `log4j-smtp` artifact
>> of type POM, which would pull those dependencies without the
>> "optional" part. Just to prepare users for Log4j 3.x.
>>
>> Piotr
>>
Re: Jakarta based appenders
Posted by Gary Gregory <ga...@gmail.com>.
IMO we should not do any reflection magic, you just say which module you
want, nice and simple. Internally, this might mean we alias the appender
names just like we did for mongodb Appenders when MongoDb 4 came out. I'm
not sure what the names should be in this case. Prefixing Appender names
with Jakarata or JEE/Jee is fine w me but unlikely to please others, as
would renaming the old Appender to something else.
Gary
Gary
On Sat, Mar 5, 2022, 03:31 Piotr P. Karwasz <pi...@gmail.com> wrote:
> Hello,
>
> A month ago Andy Wilkinson (from Spring Boot I presume) raised the
> issue of the lack of support in Log4j 2.x for the Jakarta version of
> Java Mail (LOG4J2-3362).
>
> In Log4j 3.x adding such a feature is trivial (copy 'log4j-smtp`
> artifact, run Tomcat's conversion tool and update the dependencies),
> but I am not sure if Log4j 3.x will be released in time for Spring
> Boot 3.x (which has a baseline of Jakarta EE 9).
>
> In Log4j 2.x both the SMTP and JMS appenders are in `log4j-core`,
> which complicates the situation. A month ago I submitted:
>
> https://github.com/apache/logging-log4j2/pull/745
>
> which was unacceptable, because it added the Jakarta SMTP manager
> directly to `log4j-core` and added new dependencies.
>
> Thanks to your objections I refurbished it with these assumptions:
>
> * the Java EE SMTP appender and manager stay in `log4j-core`,
> * a new 'log4j-jakarta-smtp` artifact is introduced, which only
> contains the manager part of the Jakarta EE SMTP appender, so that
> users can still use the "SMTP" plugin name in their configuration,
> * everything is glued together using reflection: if the SMTP appender
> (in `log4j-core`) detects the presence of `log4j-jakarta-smtp` it
> switches to the Jakarta manager. There is of course also a manual
> on/off switch.
>
> Can someone have a look at the PR, whether it is acceptable?
>
> With Java Mail there is also the problem of which dependencies to
> declare: there is a separate artifact for the Java Mail API, but the
> `com.sun.mail` implementation artifacts include the API classes. There
> are no API-less artifacts as in the case of JAXB (the `org.glassfish`
> artifacts). Therefore I reconfigured the dependencies of `log4j-core`
> to include:
>
> * the Java Activation and Mail APIs as optional compile dependencies
> (should it be 'provided'?),
> * the implementation 'java.mail' artifact as runtime dependency.
>
> Does it seem Ok? We could also create a separate `log4j-smtp` artifact
> of type POM, which would pull those dependencies without the
> "optional" part. Just to prepare users for Log4j 3.x.
>
> Piotr
>