ppkarwasz commented on code in PR #2249:
URL: https://github.com/apache/logging-log4j2/pull/2249#discussion_r1469336334
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/jmx/StatusLoggerAdmin.java:
##########
@@ -92,6 +80,50 @@ private static MBeanNotificationInfo
createNotificationInfo() {
return new MBeanNotificationInfo(notifTypes, name, description);
}
+ /// BEGIN: Conditional `StatusListener` registration
///////////////////////////////////////////////////////////////
+
+ // `StatusLogger` contains a _fallback listener_ that defaults to a
`StatusConsoleListener`.
+ // It is used to propagate logs when no listeners are available.
+ // If JMX registers itself always, unconditionally, this would render the
fallback (console) listener ineffective.
+ // That is, no status logs would be written to console when
`StatusLoggerAdmin` is in the classpath.
+ // To avoid this undesired behaviour, we register JMX status listener only
when there is a party interested in these
+ // notifications.
+
+ @Override
+ public void addNotificationListener(
+ final NotificationListener listener, final NotificationFilter
filter, final Object handback) {
+ super.addNotificationListener(listener, filter, handback);
+ registeredNotificationListeners.add(listener);
+ updateStatusListenerRegistration();
+ }
+
+ @Override
+ public void removeNotificationListener(final NotificationListener
listener) throws ListenerNotFoundException {
+ super.removeNotificationListener(listener);
+ registeredNotificationListeners.remove(listener);
+ updateStatusListenerRegistration();
Review Comment:
Looking at the `NotificationBroadcasterSupport` class, the logic to add and
remove listeners is more complex.
If I understand correctly:
* NBS remembers a list of triplets `(listener, filter, handback`,
* the `removeNotificationListener(listener)` method removes all the
triplets with the give `listener` value,
* the `removeNotificationListener(listener, filter, handback)` method
remove **only** the triplet with the given values of `listener`, `filter` and
`handback`.
I am not convinced we need to implement this logic, but we should either:
* implement the complete logic,
* or limit ourselves to registering the status listener in
`addNotificationListener` and removing the status listener only when
`MBeanRegistration#postDeregister` is called. This way we ignore all the
intricacies of notifications.
--
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-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
