commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bugzi...@apache.org
Subject DO NOT REPLY [Bug 34661] - [logging][PATCH] Improvements to LogFactoryImpl
Date Mon, 30 May 2005 15:40:33 GMT
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=34661>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=34661





------- Additional Comments From b_stansberry@hotmail.com  2005-05-30 17:40 -------
(In reply to comment #12)
> re patch 3a:
> 
> In discoverLogImplementation, when the user has specified an explicit log class
> then we shouldn't do any discovery, yes? We should just load that logging class
> or fail. But I'm not sure that is what the code is doing. If
> handleFlawedDiscovery returns, then the flow drops down into the discovery part,
> right? 

Arggh.  Good catch.  In this version handleFlawedDiscovery always throws a
LogConfigurationException, but still discoverLogImplementation shouldn't count
on that fact.

> 
> When testing for the jdk14 logger, I'm not sure that the code to check that java
> 1.4 is actually available belongs here. I'm surprised that it would be possible
> to instantiate a Jdk14Logger in a pre-1.4 JVM at all, as the class does have
> parameters with types only available in java1.4. However if java's "lazy
> linking" is so advanced that it doesn't even resolve those types until needed,
> then we can simply fix that by having a static block in Jdk14Logger that
> references Level.class or similar. It seems to me to be cleaner for Logger
> classes to be responsible for validating whether they can run or not than
> putting this in the LogFactoryImpl class.
> 
+1.

I don't like that stuff in here either, and your static initializer idea removes
any remote justification for it.

> I think there's a copy-and-paste error in the comments for the Lumberjack test
>

???
 
> Method createLogFromClass has side-effects: it sets a number of instance
> variables as well as returning the Log object. But the isXXXAvailable methods
> also call createLogFromClass - meaning that if anyone called one of those and it
> succeeded, then this would "reset" the logConstructor and related members.
> Possibly createLogFromClass could be split into two parts, with only the
> side-effect-free part being called from the isXXXAvailable methods, but I would
> prefer to simply do away with the isXXXAvailable methods completely.
> 

Haven't thought carefully but this could perhaps be handled with a simple "if
(logConstructor == null)" test around the code that sets instance variables.  I
too would prefer to see the isXXXAvailable methods go, even more so now that
you've pointed out the can have side effects.

> The comments on handleFlawedDiscovery still reference #FAILURE_PROPERTY, though
> I presume the actual code is in a different patch.

Yes.  Will fix.

Thanks for the careful review.

-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

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


Mime
View raw message