commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From <cmanola...@yahoo.com>
Subject Re: Problems with commons-logging
Date Mon, 04 Feb 2002 04:49:44 GMT
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:commons-dev-unsubscribe@jakarta.apache.org>
For additional commands, e-mail: <mailto:commons-dev-help@jakarta.apache.org>


Mime
View raw message