Jess Holle wrote:
Finally, I hope to get a chance to chase the lack of appender removal and log level change event firings and propose patches for these issues as well.Attached is a patch that adds event firing upon appender removal and log level change. Please take this patch. The only real alternative I see is to remove the appenderRemovedEvent() and levelChangedEvent() methods from LoggerEventListener as they currently advertise events that will never be fired which is very misleading. I think it is much preferable to actually fire these events, however -- especially levelChangedEvent() as that would be quite useful in some cases. I'll file a bug so this does not get lost in the shuffle as well. One coment: I note the new locking strategy in Category and this seems a lot better than that in 1.2. It looks pretty close to Java 5's locks in many respects (though I note the lack of re-entrancy, which is understandable here). That said, I also note many code sections like: lock.getWriteLock();
if ((appender == null) || (aai == null)) {
// Nothing to do
} else {
aai.removeAppender(appender);
}
lock.releaseWriteLock();I would be quite concerned about the maintainability of this code as it stands:
lock.getWriteLock();
try
{
if ((appender == null) || (aai == null)) {
// Nothing to do
} else {
aai.removeAppender(appender);
}
}
finally
{
lock.releaseWriteLock();
}
which handles both of these problems.-- Jess Holle |
Index: src/java/org/apache/log4j/Category.java
===================================================================
--- src/java/org/apache/log4j/Category.java (revision 349818)
+++ src/java/org/apache/log4j/Category.java (working copy)
@@ -1210,6 +1210,8 @@
aai.removeAppender(appender);
}
lock.releaseWriteLock();
+
+ repository.fireRemoveAppenderEvent((Logger) this, appender);
}
/**
@@ -1221,14 +1223,19 @@
* @since 0.8.2
*/
public void removeAppender(String name) {
+ Appender appender = null;
+
lock.getWriteLock();
-
if ((name == null) || (aai == null)) {
// nothing to do
} else {
- aai.removeAppender(name);
+ appender = aai.getAppender( name );
+ aai.removeAppender( appender );
}
lock.releaseWriteLock();
+
+ if ( appender != null )
+ repository.fireRemoveAppenderEvent((Logger) this, appender);
}
/**
@@ -1265,7 +1272,17 @@
* </p>
*/
public void setLevel(Level level) {
+ if ( level != null )
+ {
+ if ( level == this.level )
+ return; // same level objects
+ if ( level.equals( this.level ) )
+ return; // log level is not really changing
+ }
+ else if ( this.level == null )
+ return; // both levels are null
this.level = level;
+ repository.fireLevelChangedEvent( (Logger) this );
}
// /**
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
