You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "jvz (via GitHub)" <gi...@apache.org> on 2023/10/15 23:06:56 UTC
[PR] Remove dependency on `java.desktop` (logging-log4j2)
jvz opened a new pull request, #1863:
URL: https://github.com/apache/logging-log4j2/pull/1863
This adds `ConfigurationChangeListener` and `ConfigurationChangeEvent` to replace the use of `java.beans.PropertyChangeListener` and `java.beans.PropertyChangeEvent` respectively. This removes the need for the `java.desktop` module in both log4j-core and log4j-jul.
## Checklist
* Base your changes on `2.x` branch if you are targeting Log4j 2; use `main` otherwise
* `./mvnw verify` succeeds (if it fails due to code formatting issues reported by Spotless, simply run `./mvnw spotless:apply` and retry)
* Non-trivial changes contain an entry file in the `src/changelog/.2.x.x` directory
* Tests for the changes are provided
* [Commits are signed](https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits) (optional, but highly recommended)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove dependency on `java.desktop` (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on PR #1863:
URL: https://github.com/apache/logging-log4j2/pull/1863#issuecomment-1764932276
> Looks good to me.
>
> If we are introducing a breaking change in `LoggerContext`, I would also reevaluate the role of [`ConfigurationListener`](https://github.com/apache/logging-log4j2/blob/main/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationListener.java). I have the impression that the two kinds of listeners have a very similar role and can be merged (of `ConfigurationListener` can be dropped).
>
> The role of `ConfigurationListener` is just to inform `LoggerContext` that the configuration file changed and trigger a reconfiguration. I don't think any other component really uses this interface: most components should wait for a `ConfigurationChangedEvent`.
I agree that these two should probably be combined.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove dependency on `java.desktop` (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #1863:
URL: https://github.com/apache/logging-log4j2/pull/1863#issuecomment-1784651407
@jvz,
Thanks, `java.desktop` is one of the largest modules. The uncompressed sizes of our `log4j-samples-jlink` image are:
```
173668 .
40736 ./java.base
35736 ./java.desktop
16816 ./java.xml
7192 ./org.apache.logging.log4j.core
2924 ./java.management
1256 ./org.apache.logging.log4j
928 ./java.rmi
632 ./java.compiler
392 ./java.logging
200 ./java.prefs
160 ./java.datatransfer
124 ./java.scripting
32 ./org.apache.logging.log4j.samples.jlink
```
(sizes in KiB). This means we have reduced that image by one third.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove dependency on `java.desktop` (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #1863:
URL: https://github.com/apache/logging-log4j2/pull/1863#discussion_r1360384294
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java:
##########
@@ -711,7 +707,8 @@ public Configuration setConfiguration(final Configuration config) {
prev.stop();
}
- firePropertyChangeEvent(new PropertyChangeEvent(this, PROPERTY_CONFIG, prev, config));
+ final var event = new ConfigurationChangeEvent(this, prev, config);
+ configurationChangeListeners.forEach(listener -> listener.onChange(event));
Review Comment:
Probably we should add a `try ... catch` block to prevent bugs in the listeners from breaking Log4j.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove dependency on `java.desktop` (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz merged PR #1863:
URL: https://github.com/apache/logging-log4j2/pull/1863
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove dependency on `java.desktop` (logging-log4j2)
Posted by "jvz (via GitHub)" <gi...@apache.org>.
jvz commented on code in PR #1863:
URL: https://github.com/apache/logging-log4j2/pull/1863#discussion_r1361012895
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java:
##########
@@ -711,7 +707,8 @@ public Configuration setConfiguration(final Configuration config) {
prev.stop();
}
- firePropertyChangeEvent(new PropertyChangeEvent(this, PROPERTY_CONFIG, prev, config));
+ final var event = new ConfigurationChangeEvent(this, prev, config);
+ configurationChangeListeners.forEach(listener -> listener.onChange(event));
Review Comment:
I suppose that makes sense as an improvement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org