On 8 Jul 2008, at 18:25, Galder Zamarreno wrote: > Hi all, > > Yesterday while looking at the JBC code, I spotted that > CommandInterceptor caches log.isTraceEnabled() as instance variable > and that pretty much all classes extending this use it. > > In a standalone world, where JBC runs on its own this is fine. > However, when running within AS, log4j settings could change at > runtime (conf/jboss-log4j.xml is hot deployable) and therefore, > someone could enable TRACE logging for org.jboss.cache at runtime > which wouldn't have any effect with the current JBC code. One would think that changing the log level all the way down to TRACE would not really happen on a live system so a restart would be ok. We don't do this for any other log level, just TRACE. > As far as I can remember, it's generally recommended not to cache > things like this in code running within AS. Then again, I can see how > convenient caching such value is rather than calling > log.isTraceEnabled() all the time. I doubt using an instance variable > or calling log.isTraceEnabled() would have a real impact performance > wise when TRACE is not enabled. No, log.isTraceEnabled() can be pretty expensive. :-) > Thinking about this, I wonder whether any callback(s) could be > implemented when the log4j settings are updated after a hot deployment > and within this callback, update interceptors' trace settings... Wouldn't that tie JBC to log4j? I currently just use commons-logging. -- Manik Surtani Lead, JBoss Cache manik(a)jboss.org

By the way, with the email below, I'm assuming that there's a least a few of the CommandInterceptor instances that are created at runtime and the same instance is used throughout all cache lifespan. I dunno whether any of this CommandInterceptor instances are created on a per invocation basis. If they are, they can safely cache log.isTraceEnabled() as the duration of the CommandInterceptor instance is short. I doubt this is the case though.