BTW, another issue I just saw:
catch(Throwable) {}
Is it really the intent to catch OutOfMemoryError, and do nothing?
Or, you really want to keep the thread from cleaning up when thread.stop()
is called (ThreadDeath)?
Throwable is not shorthand for "list of exceptions I mean to catch".
Catching Throwable means "I'll handle every possible thing that goes wrong,
up to and including VM bugs, like InternalError"
Just because it seems unlikely to happen in the block of code you are
calling, doesn't mean that it won't eventually happen.
Inline:
> -----Original Message-----
> From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]]
> Sent: Sunday, February 03, 2002 11:50 PM
> To: Jakarta Commons Developers List
> Subject: Re: Problems with commons-logging
>
>
> 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 ).
>
That's got to be the business of the logging package, and commons-logging is
not a logging package, it's an interface to logging packages.
Commons-logging shouldn't make any claims or guarantees about what the
logging package does.
>
> > > - 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.
>
The advantage of interfaces over abstract classes in Java is that interfaces
may be multiply inherited from. This doesn't seem to be an issue here. I
don't see any XLogImpl classes that need to mix in the commons logging
interface. The downside is that the static will have to have an
implementation. If that forwards to LogSource (LogManager?), it does reduce
the surface complexity of commons logging a bit. Most components will not
need LogSource at all.
>
> > > - 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.
>
>
I think that log(int level,...) is a bad idea. Where do those ints come
from? If they are the levels defined by the logging package (ie logkit,
log4j, jdk, ...) then components can NOT use them. Not without having random
behavior when packages are changed. If they are constants that are defined
by the commons logging package, you've just added a new mapping problem. Or
it's just a switch statement that dispatches to {trace(), debug(), info(),
...}, which doesn't seem to add anything.
> > > - 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 ?
>
commons-X will not require or hardcode log4j or logkit. Application Z, that
uses commons-X will require or hardcode log4j or logkit or something else.
Will Tomcat hardcode or require some logging package? Possibly. That's up to
Tomcat. But, SO WHAT? It's not as though that will be exposed to webapps
running within Tomcat. It's Tomcat's business how log information is
produced. So long as log information IS produced. Does anyone care what
syslog package httpd uses?
--
To unsubscribe, e-mail: <mailto:[EMAIL PROTECTED]>
For additional commands, e-mail: <mailto:[EMAIL PROTECTED]>
