Re: [logging-log4j2] branch master updated: [LOG4J2-3459] - Add LoggingSystemProvider SPI
I have more questions on this.
1. PropertiesUtil cannot be deprecated. It is used by Spring Boot to register
its PropertiesSource.
2. All these classes that you have marked internal are opened to various Log4j
modules. That means they are NOT open to any other implementation of the
Log4j-API. Can we really do that?
The more I am looking at this I would prefer that you revert this commit and
put it on a branch as several PRs that split it up into things we can really
review.
So I am -1 on this commit.
Ralph
> On Oct 31, 2022, at 7:40 PM, Apache wrote:
>
> Are you using your old email address? I keep having to moderate them through.
>
> Yes they should be in an appropriately named package under the internal
> package.
>
> Ralph
>
>> On Oct 31, 2022, at 7:36 PM, Matt Sicker wrote:
>>
>> We could consolidate the Log4j-private classes into the internal package
>> and export it to other Log4j modules. That makes sense.
>>
>> And LoggingSystem probably doesn’t need a Lazy instance of itself, right.
>>
>> —
>> Matt Sicker
>>
>>> On Oct 31, 2022, at 20:58, Apache wrote:
>>>
>>>
>>>
>>> See below
>>>
> On Oct 31, 2022, at 6:49 PM, Matt Sicker
> wrote:
The util3 package is a collection of all the private classes we have. We
discussed the general idea on a recent conference call. These are pseudo
private classes. They’re meant for internal use only and are guarded by
the module info. JUnit does the same thing
>>>
>>> If they are private then they should be under a package named internal. It
>>> won’t be obvious to devs not using JPMS that they are private.
>>>
The point of Lazy is ensuring thread safe publication of initialized
variables that may be initialized by multiple threads. There are two main
variants: one that guarantees the supplier function is executed only once
and one that may run the supplier more than once but has a single return
value.
>>>
>>> But why is it necessary? When getInstance is called that operation should
>>> only be performed once. GetInstance is already lazy so why do we need lazy
>>> initialization of the stuff in it?
>>>
>>> Ralph
>>>
—
Matt Sicker
>> On Oct 31, 2022, at 17:53, Ralph Goers
>> wrote:
>
> Also, in LoggingSystem.java
>
> private static final String[] COMPATIBLE_API_VERSIONS = {"2.6.0"};
>
> absolutely needs to be changed to version “3.0.0”.
>
> Ralph
>
>>> On Oct 31, 2022, at 3:49 PM, Ralph Goers
>>> wrote:
>>
>> Matt,
>>
>> I absolutely would have preferred that you had created a PR for this and
>> asked for a review before committing it as I have a few concerns:
>>
>> 1. It is a massive commit. It is going to take time to digest exactly
>> what you have done.
>> 2. You have created a util3 package in API with no discussion of this on
>> the dev list. I assume that it contains artifacts that were cleaned up
>> from util and are now no longer compatible? Or does it only contain
>> classes that are exportable to other Log4j artifacts (I do see that in
>> module-info.java). If these classes are meant to be private then I would
>> have preferred that it be moved to internal/util.
>> 3. I no longer understand the point of Lazy<>. LogManager now performs
>> no static initialization that I can see. Instead, it calls
>> LoggingSystem.getInstance(). What is the point of making initialization
>> of INSTANCE lazy? You accomplished what you wanted to by moving the
>> static initialization to another class.
>> 4-10. I have no idea as it will take me a week to muddle through the
>> rest of all these changes to figure out what happened.
>>
>> Ralph
>>
On Oct 30, 2022, at 6:24 PM, mattsic...@apache.org wrote:
>>>
>>> This is an automated email from the ASF dual-hosted git repository.
>>>
>>> mattsicker pushed a commit to branch master
>>> in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
>>>
>>>
>>> The following commit(s) were added to refs/heads/master by this push:
>>> new aab23d5c05 [LOG4J2-3459] - Add LoggingSystemProvider SPI
>>> aab23d5c05 is described below
>>>
>>> commit aab23d5c05bff2915c290dff3e73a0ae03caf43a
>>> Author: Matt Sicker
>>> AuthorDate: Sun Oct 30 20:24:17 2022 -0500
>>>
>>> [LOG4J2-3459] - Add LoggingSystemProvider SPI
>>>
>>> This helps clean up a lot of static state in Log4j API along with
>>> enabling further extensibility by exposing
>>> LoggingSystemProvider::getInstance which can be implemented using
>>> Injector in Log4j Core. The general utility class for working with this
>>> provider is available as an internal (util3) class in LoggingSystem.
>>> Test fixture helper classes have also been updated to use LoggingSystem
>>>
Re: [logging-log4j2] branch master updated: [LOG4J2-3459] - Add LoggingSystemProvider SPI
Mail client selected the wrong email, whoops. Anyways, I’ll revert and break up
into more commits and put the more interesting changes into a PR.
—
Matt Sicker
> On Nov 1, 2022, at 10:21, Ralph Goers wrote:
>
> I have more questions on this.
>
> 1. PropertiesUtil cannot be deprecated. It is used by Spring Boot to register
> its PropertiesSource.
> 2. All these classes that you have marked internal are opened to various
> Log4j modules. That means they are NOT open to any other implementation of
> the Log4j-API. Can we really do that?
>
> The more I am looking at this I would prefer that you revert this commit and
> put it on a branch as several PRs that split it up into things we can really
> review.
>
> So I am -1 on this commit.
>
> Ralph
>
>> On Oct 31, 2022, at 7:40 PM, Apache wrote:
>>
>> Are you using your old email address? I keep having to moderate them through.
>>
>> Yes they should be in an appropriately named package under the internal
>> package.
>>
>> Ralph
>>
On Oct 31, 2022, at 7:36 PM, Matt Sicker wrote:
>>>
>>> We could consolidate the Log4j-private classes into the internal package
>>> and export it to other Log4j modules. That makes sense.
>>>
>>> And LoggingSystem probably doesn’t need a Lazy instance of itself, right.
>>>
>>> —
>>> Matt Sicker
>>>
On Oct 31, 2022, at 20:58, Apache wrote:
See below
>> On Oct 31, 2022, at 6:49 PM, Matt Sicker
>> wrote:
>
> The util3 package is a collection of all the private classes we have. We
> discussed the general idea on a recent conference call. These are pseudo
> private classes. They’re meant for internal use only and are guarded by
> the module info. JUnit does the same thing
If they are private then they should be under a package named internal. It
won’t be obvious to devs not using JPMS that they are private.
> The point of Lazy is ensuring thread safe publication of initialized
> variables that may be initialized by multiple threads. There are two main
> variants: one that guarantees the supplier function is executed only once
> and one that may run the supplier more than once but has a single return
> value.
But why is it necessary? When getInstance is called that operation should
only be performed once. GetInstance is already lazy so why do we need lazy
initialization of the stuff in it?
Ralph
>
> —
> Matt Sicker
>
>>> On Oct 31, 2022, at 17:53, Ralph Goers
>>> wrote:
>>
>> Also, in LoggingSystem.java
>>
>> private static final String[] COMPATIBLE_API_VERSIONS = {"2.6.0"};
>>
>> absolutely needs to be changed to version “3.0.0”.
>>
>> Ralph
>>
On Oct 31, 2022, at 3:49 PM, Ralph Goers
wrote:
>>>
>>> Matt,
>>>
>>> I absolutely would have preferred that you had created a PR for this
>>> and asked for a review before committing it as I have a few concerns:
>>>
>>> 1. It is a massive commit. It is going to take time to digest exactly
>>> what you have done.
>>> 2. You have created a util3 package in API with no discussion of this
>>> on the dev list. I assume that it contains artifacts that were cleaned
>>> up from util and are now no longer compatible? Or does it only contain
>>> classes that are exportable to other Log4j artifacts (I do see that in
>>> module-info.java). If these classes are meant to be private then I
>>> would have preferred that it be moved to internal/util.
>>> 3. I no longer understand the point of Lazy<>. LogManager now performs
>>> no static initialization that I can see. Instead, it calls
>>> LoggingSystem.getInstance(). What is the point of making initialization
>>> of INSTANCE lazy? You accomplished what you wanted to by moving the
>>> static initialization to another class.
>>> 4-10. I have no idea as it will take me a week to muddle through the
>>> rest of all these changes to figure out what happened.
>>>
>>> Ralph
>>>
> On Oct 30, 2022, at 6:24 PM, mattsic...@apache.org wrote:
This is an automated email from the ASF dual-hosted git repository.
mattsicker pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
The following commit(s) were added to refs/heads/master by this push:
new aab23d5c05 [LOG4J2-3459] - Add LoggingSystemProvider SPI
aab23d5c05 is described below
commit aab23d5c05bff2915c290dff3e73a0ae03caf43a
Author: Matt Sicker
AuthorDate: Sun Oct 30 20:24:17 2022 -0500
[LOG4J2-3459] - Add LoggingSystemProvider SPI
This helps clean up a lot of static state in Log4j API along with
enabling
Re: [logging-log4j2] branch master updated: [LOG4J2-3459] - Add LoggingSystemProvider SPI
I’ve reverted the last two commits as they were basically the last commit,
though I’m not exactly sure if you’re referring to that one in particular. I’ll
break it down into smaller pieces, though, and submit the non-trivial parts as
a PR.
—
Matt Sicker
> On Nov 1, 2022, at 10:21, Ralph Goers wrote:
>
> I have more questions on this.
>
> 1. PropertiesUtil cannot be deprecated. It is used by Spring Boot to register
> its PropertiesSource.
> 2. All these classes that you have marked internal are opened to various
> Log4j modules. That means they are NOT open to any other implementation of
> the Log4j-API. Can we really do that?
>
> The more I am looking at this I would prefer that you revert this commit and
> put it on a branch as several PRs that split it up into things we can really
> review.
>
> So I am -1 on this commit.
>
> Ralph
>
>> On Oct 31, 2022, at 7:40 PM, Apache wrote:
>>
>> Are you using your old email address? I keep having to moderate them through.
>>
>> Yes they should be in an appropriately named package under the internal
>> package.
>>
>> Ralph
>>
>>> On Oct 31, 2022, at 7:36 PM, Matt Sicker wrote:
>>>
>>> We could consolidate the Log4j-private classes into the internal package
>>> and export it to other Log4j modules. That makes sense.
>>>
>>> And LoggingSystem probably doesn’t need a Lazy instance of itself, right.
>>>
>>> —
>>> Matt Sicker
>>>
On Oct 31, 2022, at 20:58, Apache wrote:
See below
>> On Oct 31, 2022, at 6:49 PM, Matt Sicker
>> wrote:
>
> The util3 package is a collection of all the private classes we have. We
> discussed the general idea on a recent conference call. These are pseudo
> private classes. They’re meant for internal use only and are guarded by
> the module info. JUnit does the same thing
If they are private then they should be under a package named internal. It
won’t be obvious to devs not using JPMS that they are private.
> The point of Lazy is ensuring thread safe publication of initialized
> variables that may be initialized by multiple threads. There are two main
> variants: one that guarantees the supplier function is executed only once
> and one that may run the supplier more than once but has a single return
> value.
But why is it necessary? When getInstance is called that operation should
only be performed once. GetInstance is already lazy so why do we need lazy
initialization of the stuff in it?
Ralph
>
> —
> Matt Sicker
>
>>> On Oct 31, 2022, at 17:53, Ralph Goers
>>> wrote:
>>
>> Also, in LoggingSystem.java
>>
>> private static final String[] COMPATIBLE_API_VERSIONS = {"2.6.0"};
>>
>> absolutely needs to be changed to version “3.0.0”.
>>
>> Ralph
>>
On Oct 31, 2022, at 3:49 PM, Ralph Goers
wrote:
>>>
>>> Matt,
>>>
>>> I absolutely would have preferred that you had created a PR for this
>>> and asked for a review before committing it as I have a few concerns:
>>>
>>> 1. It is a massive commit. It is going to take time to digest exactly
>>> what you have done.
>>> 2. You have created a util3 package in API with no discussion of this
>>> on the dev list. I assume that it contains artifacts that were cleaned
>>> up from util and are now no longer compatible? Or does it only contain
>>> classes that are exportable to other Log4j artifacts (I do see that in
>>> module-info.java). If these classes are meant to be private then I
>>> would have preferred that it be moved to internal/util.
>>> 3. I no longer understand the point of Lazy<>. LogManager now performs
>>> no static initialization that I can see. Instead, it calls
>>> LoggingSystem.getInstance(). What is the point of making initialization
>>> of INSTANCE lazy? You accomplished what you wanted to by moving the
>>> static initialization to another class.
>>> 4-10. I have no idea as it will take me a week to muddle through the
>>> rest of all these changes to figure out what happened.
>>>
>>> Ralph
>>>
> On Oct 30, 2022, at 6:24 PM, mattsic...@apache.org wrote:
This is an automated email from the ASF dual-hosted git repository.
mattsicker pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
The following commit(s) were added to refs/heads/master by this push:
new aab23d5c05 [LOG4J2-3459] - Add LoggingSystemProvider SPI
aab23d5c05 is described below
commit aab23d5c05bff2915c290dff3e73a0ae03caf43a
Author: Matt Sicker
AuthorDate: Sun Oct 30 20:24:17 2022 -0500
[LOG4J2-3459] - Add LoggingSystemProvider SPI
>
Re: [logging-log4j2] branch master updated: [LOG4J2-3459] - Add LoggingSystemProvider SPI
Thanks Matt. I was referring to the one in the first email of this thread.
Ralph
> On Nov 1, 2022, at 1:41 PM, Matt Sicker wrote:
>
> I’ve reverted the last two commits as they were basically the last commit,
> though I’m not exactly sure if you’re referring to that one in particular.
> I’ll break it down into smaller pieces, though, and submit the non-trivial
> parts as a PR.
> —
> Matt Sicker
>
>> On Nov 1, 2022, at 10:21, Ralph Goers wrote:
>>
>> I have more questions on this.
>>
>> 1. PropertiesUtil cannot be deprecated. It is used by Spring Boot to
>> register its PropertiesSource.
>> 2. All these classes that you have marked internal are opened to various
>> Log4j modules. That means they are NOT open to any other implementation of
>> the Log4j-API. Can we really do that?
>>
>> The more I am looking at this I would prefer that you revert this commit and
>> put it on a branch as several PRs that split it up into things we can really
>> review.
>>
>> So I am -1 on this commit.
>>
>> Ralph
>>
>>> On Oct 31, 2022, at 7:40 PM, Apache wrote:
>>>
>>> Are you using your old email address? I keep having to moderate them
>>> through.
>>>
>>> Yes they should be in an appropriately named package under the internal
>>> package.
>>>
>>> Ralph
>>>
On Oct 31, 2022, at 7:36 PM, Matt Sicker wrote:
We could consolidate the Log4j-private classes into the internal package
and export it to other Log4j modules. That makes sense.
And LoggingSystem probably doesn’t need a Lazy instance of itself, right.
—
Matt Sicker
> On Oct 31, 2022, at 20:58, Apache wrote:
>
>
>
> See below
>
>>> On Oct 31, 2022, at 6:49 PM, Matt Sicker
>>> wrote:
>>
>> The util3 package is a collection of all the private classes we have.
>> We discussed the general idea on a recent conference call. These are
>> pseudo private classes. They’re meant for internal use only and are
>> guarded by the module info. JUnit does the same thing
>
> If they are private then they should be under a package named internal.
> It won’t be obvious to devs not using JPMS that they are private.
>
>> The point of Lazy is ensuring thread safe publication of initialized
>> variables that may be initialized by multiple threads. There are two
>> main variants: one that guarantees the supplier function is executed
>> only once and one that may run the supplier more than once but has a
>> single return value.
>
> But why is it necessary? When getInstance is called that operation should
> only be performed once. GetInstance is already lazy so why do we need
> lazy initialization of the stuff in it?
>
> Ralph
>
>>
>> —
>> Matt Sicker
>>
On Oct 31, 2022, at 17:53, Ralph Goers
wrote:
>>>
>>> Also, in LoggingSystem.java
>>>
>>> private static final String[] COMPATIBLE_API_VERSIONS = {"2.6.0"};
>>>
>>> absolutely needs to be changed to version “3.0.0”.
>>>
>>> Ralph
>>>
> On Oct 31, 2022, at 3:49 PM, Ralph Goers
> wrote:
Matt,
I absolutely would have preferred that you had created a PR for this
and asked for a review before committing it as I have a few concerns:
1. It is a massive commit. It is going to take time to digest exactly
what you have done.
2. You have created a util3 package in API with no discussion of this
on the dev list. I assume that it contains artifacts that were cleaned
up from util and are now no longer compatible? Or does it only contain
classes that are exportable to other Log4j artifacts (I do see that in
module-info.java). If these classes are meant to be private then I
would have preferred that it be moved to internal/util.
3. I no longer understand the point of Lazy<>. LogManager now performs
no static initialization that I can see. Instead, it calls
LoggingSystem.getInstance(). What is the point of making
initialization of INSTANCE lazy? You accomplished what you wanted to
by moving the static initialization to another class.
4-10. I have no idea as it will take me a week to muddle through the
rest of all these changes to figure out what happened.
Ralph
>> On Oct 30, 2022, at 6:24 PM, mattsic...@apache.org wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> mattsicker pushed a commit to branch master
> in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
>
>
> The following commit(s) were added to refs/heads/master by this push:
> new aab23d5c05 [LOG4J2-3459] - Add LoggingSystemProvi
