Re[2]: *LoggerManager changes

[Anton Tagunov <at...@apache.org>]

Hi, Leo!

= Changes To LoggerManager =

AT> What would you say to the following changes to
AT> *LoggerManager:
AT>
AT> 1) The creator of LoggerManager passes an initial
AT>    logger to it.

LS> yeppo! I am wondering though how much logging the LoggerManager actually 
LS> needs to do before initialization. And hence wondering what would be 
LS> actually logged to this initial logger. Keep in mind that the client of 
LS> LoggerManager should not ask it to do anything until it is initialize()d 
LS> (per avalon-framework contracts). So the only messaging left is that 
LS> from the LoggerManager itself.

Exactly! All it currently prints out, I guess are
[DEBUG] added foo
[DEBUG] added bar
but in case anything goes wrong with configuration
the warnings and errors will also go there.

LS> And that is normally served best by implementing LogEnabled.

Yes :-)
The implementation I'm about to check in does both - accepts
in the constructor and accepts enableLogging().

This all has been done before, but there's a difference:
currenlty Fortress _first_ gets a Hierarchy(), then
gets from _this_ hierarchy a logger and passes it into
LogKitLoggerManager together with this hierarchy.
So, in fact LogKitLoggerManager while configuring itself
logs to loggers extracted from the very same hierarchy
it is configuring!

Sounds a bit complicated, but so it is.
It works because with LogKit it is possible first to
get a logger (by default logging to System.out) and
_later_ set LogTargets for it.

But this results in LogKitLoggerManager having a
logger logging to System.out to log its own messages
while it is "warming up".

What I propose to change is that in Fortress for instance
DefaultContainerManager will no longer pass a logger extracted
from the uninitialized Hierarchy but it will pass in its
own m_logger. The later has been supplied by the user
and will be in servlet environment, for instance, ServletLogger.
This way we shall get rid of any chance that anything will
go to the console when we don't want it too (servlets, other
manager environments, etc.)

One more thing is that this trick (first creating a logger
and passing it to LoggerManager via enableLogging() and
_then_ configuring it) won't work with Log4J.

My approach will work uniformly.


AT> 2)     XXXLoggerManager(
AT>                String prefix,
AT>                Logger initialLogger,
AT>                String switchToCategory )

And the last parameter here is in order to
_after_ we have configured ourselves get a logger
from ouselves and use it for our own logging
(being on guard not to recurse :)

AT> 5) But if the hierarchy has been passed to us already created,
AT>    should we do _any_ of this?

LS> I think: not. The general assumption I guess is that the entity (usually 
LS> a piece of a container) passing in the hierarchy has intimate knowledge 
LS> of what should happen.

Yes, I think it's better.

AT> 6)
AT>    There's an option to test if our logging is alive
AT>    in the end of configure() method by doing
AT>
AT>        getLoggerForCategory("").info("Logging started");
AT>
AT>    Please, give me an advice on this feature.

LS> not knowing the code well, this seems like a weird way to test if you 
LS> can log.

So it seems to me. Okay, looks better we shouldn't do it.

AT> 7) LogKitLoggerManager(
AT>            String prefix,
AT>            Logger defaultLogger,
AT>            Logger interanlLogger )
AT>    {
AT>        m_defaultLogger = defaultLogger;
AT>        ...
AT>    }
AT>
AT>    Logger getDefaultLogger()
AT>    {
AT>        return m_defaultLogger;
AT>    }
AT>
AT>    I must confess I sort of dislike this.

LS> hmm, to me it sounds rather sensible & predicatble :D
err.. my dislikings or the code? :-)

AT>    With regards to this issue I would consider the following
AT>    options:
AT>
AT>    a) remove this constructor completely

LS> can't do that. This is released code, after all!
ok

AT>    b) deprecate this constructor but retain functionality
AT>    c) leave the constructor undeprecated
AT>
AT>    I would like an advice on this, I just do not dare to
AT>    do a) and b) without a permit, although I feel very much
AT>    inclined to do one of this.

LS> perhaps you can explain a little more just why this constructur sucks so 
LS> bad....

We're configuring _all_ our logging system from a configuration file.
Why do we want to override what the config file has configured for
the "" category programmatically? Let the user choose the logger
for "", let him set it up in the configuration file. And if _he_
wants the same kind of logger that was used before, say console
or servlet let him configure it there. I can not imagine
circumstances when we would like to take that freedom away from
user.

Besides the inline comments in LoggerManager.java state that
    getDefaultLogger() == getLoggerForCategory("")
and it looks quite natural and predicatable, does not it?

Overriding the default logger by force via this constructor
will break this constraint, which is not good, IMO


LS> I can imagine valid use cases. Furthermore, I can imagine it 
LS> actually being used in places.

Hmm.. it is surely used in Fortress but only to overcome the
    getDefaultLogger() != getLoggerForCategory("")
issue. With my patches Fortress code will become simpler
and will still have the same effect.

Is it used for anything else?

AT> 8) LogKitLoggerManager(
AT>            String prefix,
AT>            Logger defaultLogger)
AT>
AT>    in line with changes discussed in section 7) I would
AT>    like to change the contract

LS> again, changing contracts on released code is something I'm weary of.
So am I.

LS> What use cases would you satisfy like this
I will keep true to the getDefaultLogger() == getLoggerForCategory("")
constraint. *LoggerManager will behave like all other normal
Components: when they get one logger this will be the logger they
use _only_ for themselves.

LS> and what would you break?
I do not think I will break much. Fortress did not use this
constructor at all, what other containers did use it?
The problem is that I can not come up with valid use cases
for the _old_ contract, that's why I want to change it.

= Talk First or Code First? =
LS> If there is a reasonable chance your changes might affect people in a bad
LS> way or will not be accepted because you suspect there might be a better 
LS> way to do things, it's better to talk first, commit later.
Ok, I'm trying to :)

- Anton


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@avalon.apache.org
For additional commands, e-mail: dev-help@avalon.apache.org