commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Scott Sanders <sand...@apache.org>
Subject Re: Problems with commons-logging
Date Sun, 03 Feb 2002 01:17:53 GMT
On Sat, Feb 02, 2002 at 08:33:46AM -0800, costinm@covalent.net wrote:
> Here's a list of problems I see in the current logging API.
> 
> Given that a lot of packages will eventually depend and use this API,
> I believe at least the first issues should be resolved before a release.
> 

Thanks for takeing the time to look at this Costin.  I appreciate it.

> - security: getLogNames() and getInstance() are evil and unacceptable.
> Both log4j and logkit have solutions that allow safe use in a container
> environment, i.e. support of isolation for the users of the API
> ( one app using the package can't mess with another app's logging ).
> I'm -1 on releasing with this whole in it.
> 

I can see how getLogNames() could be considered evil, and I do not see what it gains us. 
I am +1 on remving getLogNames().

Are you saying that with getInstance(), we should remove it and just use newLogInstance()?
 I am also fine with this, albeit a +0.


> - 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?

> - 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.

 
> - 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?.
 
> - 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?
 
> - 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.
 
> - I don't like the idea of constructors with a  param. All other APIs
> use a no-param constructor. You can easily call a setter if you need to.
>

Fine.  Done. +1
 
> - 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.
 
> - 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?
 
> - 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.
 
> - makeNewLogInstance comment and impl are completely of sync.
>

I will look into this.
 
> - 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'.
 

Thanks again for your thoughtful analysis.  I will look into the things that I listed.

> 
> Costin
> 
-- 
Scott Sanders - sanders@apache.org

--
To unsubscribe, e-mail:   <mailto:commons-dev-unsubscribe@jakarta.apache.org>
For additional commands, e-mail: <mailto:commons-dev-help@jakarta.apache.org>


Mime
View raw message