On Sat, 2 Feb 2002, Scott Sanders wrote: > Are you saying that with getInstance(), we should remove it and just > use newLogInstance()? I am also fine with this, albeit a +0.
It's not a naming issue, but a behavior issue. The case: you have 2 apps you want to keep isolated. Allowing one to log into the other's log is unacceptable. Classloader tricks are not allways possible and are extremely error prone ( and I would say - ineffective, can be tricked ). And the security manager is not something most people understand. We should at minimum require that getInstance( name ) would get you a logger _local_ to your app, if in a container env. ( impl: use thread class loader, or the class loader as a namespace qualifier ). > > - static methods in LogSource. I suppose LogSource is a sort of > > factory - the pattern used here is completely unnatural ( or at least > > different from most APIs in use ). > > > > Why don't we rename LogSource to LogFactory, and change > makeNewLogInstance() to newInstance()? +1 on this as well. > I believe this would be consistent with things like JAXP, correct? My naming preference would be LogManager ( as it'll not be only a Factory ). But the problem is not the name here, but the use of static methods. If we are to have a static method, it should just be a shortcut for finding a manager/factory instance and calling a virtual method. > > - I would prefer Log to be an abstract class or even to be a > > normal class, with the minimal logger - and have LogSource return > > a particular impl. If static methods are used, it's cleaner to put > > them in Log, and let the LogSource ( I would rename it LogManager ) > > be used behind the scene. I.e. > > Log log=Log.getLog( name ); > > and getLog() will find a LogManager, etc. > > > > I don't see the problem with having a factory take care of this, so I > am -0. I like that Log is an interface. What does it gives you ? Most cases will use delegation anyway. Having it as an abstract method, with a static getLog() method will make the user interact with a single class - Log - instead of 2. The LogManager/LogFactory will do the work 'behind the scene'. Nothing else change in the code ( except replacing implements with extends ). Except maybe simplifying some adapters by keeping common stuff in the base class. > > - It's missing a log() method that takes a level parameter. > > Having 5 fixed levels is fine for most apps, but not extensible enough. > > Most loggers provide such a thing. > > > > I will code this up. Thanks for the comment. +1. I am assuming void > log(int level, Object message) and the proper Throwable sister method?. It's not a big priority - it can be done after the first release. But combined with the previous point - it can make the adapter as simple as overriding the log() method and supporting the base levels. > > - also in the 'container use' case, given that the Logger will > > probably be used by many components it's likely it'll end up at top > > level loader. It would be important for different apps to use different > > logger adapters if they want to - the current solution allow only > > one. > > > > Why would this end up in the top level loader? I do not understand > this. Could you explain more please? If commons is used everywhere, it may be used for example by the Main class of tomcat ( or ant, etc ). It'll then be loaded by the parent class loader - and the static fields will be set there. You can have a CL that brakes delegation ( with all the secondary effects ), but that's not allwasy possible or desirable and it can create a huge maintainance problem. > > - Given that it is a facade, it would need some way to pass at least > > config info to the underlying logger ( at least setProperty like ). > > Some logger may not need any, but if they do it'll require using > > internal APIs. > > > > I am -1 on walking the config line. No config. None. This API intends > to mask all of this and allow a component to just log. The container > using the component will be required to configure logging. We are not > trying to replace LogKit/Log4J, we are only trying to replace > System.out.println(), IMHO. Besides, that is orhtogonal IMHO, and if > logging does this, it can do this in a later release, on a separate > interface. Yes, but we should at least allow the user to pass a simple 'config file' or base information to the real logger. Like attributes in servlet, jaxp, etc. Otherwise we'll still have to code against Log4j APIs ( to set the config file for example ) - and what's the point of using commons logging if we already require and hardcode log4j ? The default config mechanism is not allways desirable ( at least for log4j). The user will still use a log-specific config file, but at least the location of the file could be passed ( I hate system properties and props loaded from CLASSPATH ). > > - pluggable mechanism for finding the impl. Right now everything is > > hardcoded. Reading a properties from CLASSPATH or a similar mechanism > > is needed. ( jaxp style preferable - i.e. java services manifest ) > > > > It IS pluggable. Just set the org.apache.commons.logging.log property > with your Log impl. Could you code up the jar services manifest code to > augment this? Thanks. What jaxp does is to 'guess' what logger is _available_, without any user configuration. Same is done by the Class.forName() you use ( so part of the behavior is there ), but that's hard to extend without changing the code and adding more ifs. > > - Separation of interface and impl - the 2 classes that 'matter' > > are Log and LogSource, everything else should be in a different package. > > It'll get messy long term, and it's harder to read. > > > > -0. I don't really see the need to break this up. I agree that only 2 > classes really 'matter', but I do not see that as a reason to move them. > If someone else wants to, I wont stop them. Are you suggesting > something like an o.a.c.l.impl package? 1. It's harder to read ( for people who don't know the code ). A project has 100s of files, it's better to keep things organized. 2. It'll grow messy, as more adapters are added. > > - I would prefer for the base impl to be JDK1.1 compatible. There is > > no valid reason to exclude JDK1.1 usage - Hashtable can be used > > without any problem, there is nothing performance critical here. > > > > Consider this done. Note that JDK1.1 is not an absolute requirement - if we end up using the Thread.currentClassLoader() we'll loose it. But giving it up by using HashMap when Hashtable can be used is not good. > > - no support for i18n-style messages. Probably not a big deal > > for the first release, but it would be nice to think about it ( I know > > log4j can do that, I assume other as well ). > > > > -0. I personally have no intention of supporting this. I think that > commons-logging is just a simple conduit to a logging implementation, > i18n is the concern of both sides, but not the middle. If we keep the > Object as the log parameter, I don't see how we are 'missing out'. Again, for the first release we probably don't need it. It is a good idea because it makes a lot of optimizations possible and simplifies the user code and allows a more centralized management of resource bundles. I18N requires a lot of string operations - all this overhead could be eliminated by a smart logger. BTW, this would also encourage people to pass 'primary' information ( the log key and params ), which can be more valuable than a simple cooked string. Costin -- To unsubscribe, e-mail: <mailto:[EMAIL PROTECTED]> For additional commands, e-mail: <mailto:[EMAIL PROTECTED]>
