RFR: 7159567 - inconsistent configuration of MemoryHandler

Mandy Chung mandy.chung at oracle.com
Thu Oct 25 17:47:08 UTC 2012

On 10/25/2012 10:25 AM, Jim Gish wrote:
> OK.  One more time. 
> http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler

Great, thanks! I'll push it for you.

> I compromised with the RuntimeException.  I'm instead catching it, but 
> throwing a new one this way:
>    65             throw new RuntimeException(
>    66                 "Test Failed: did not load java.util.logging.ConsoleHandler as expected",
>    67                 rte);
> That way, we retain the original, but have the advantage of having a clear indication of "Test Failed" and the reason.  Otherwise, diagnosing
> the failure forces the reader to dig into the stack trace.

I like a clear error message too.  If the test fails, you will need the 
original exception and the stack trace to diagnose the problem anyway 
and in some cases, the exception is clear enough.  Anyway, just to 
explain why I had the comment.  I'm okay with what you have.


> Thanks,
>     Jim
> On 10/24/2012 08:40 PM, Mandy Chung wrote:
>> On 10/24/2012 12:31 PM, Jim Gish wrote:
>>> See updated webrev: 
>>> http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler
>> Looks good.  Thanks for the update.
>> MemoryHandlerTest.java - thanks for renaming it but you forget to 
>> change  L28 @run tag.  jtreg should fail if you run this new test.  
>> L64-66 this try-catch block isn't necessary, as I mentioned in my 
>> previous comment, but no big deal if you want to leave it there.  The 
>> comment lines and some throw statements are really long and should be 
>> broken into multiple lines (I didn't notice the long lines in 
>> previous versions - sorry if I had missed them).   Hopefully it's 
>> just one-click reformat for you using IDE :)
>> Mandy
>>> Thanks,
>>>     Jim
>>> On 10/17/2012 03:46 PM, Mandy Chung wrote:
>>>> Hi Jim,
>>>> On 10/11/2012 2:37 PM, Jim Gish wrote:
>>>>> Please review the updated changes at 
>>>>> http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler/ 
>>>> The spec change looks good.  As Alan points out, </li> is missing.  
>>>> Although they were not there before, I would think it's a good 
>>>> clean up while you are in these files if you agree.
>>> Done
>>>> The test looks better.   Is SimpleTargetHandler.numPublished 
>>>> intended to be checked?  SimpleTargetHandler is set as the target 
>>>> for java.util.logging.MemoryHandler.  I guess you want to create a 
>>>> logger using the standard MemoryHandler.
>>>> Nit: the test is named MemoryHandler and I guess the name conflict 
>>>> causes the custom handler classes to extend 
>>>> "java.util.logging.MemoryHandler" with a fully-qualified class 
>>>> name.  As the properties file is named MemoryHandlerTest.props, do 
>>>> you consider renaming the test to MemoryHandlerTest to avoid the 
>>>> name conflict?   I don't have strong opinion and just want to point 
>>>> that out.
>>> I don't see this as a problem.  However, I've renamed MemoryHandler 
>>> to MemoryHandlerTest for improved clarity.
>>>> L62-64:  better not to rethrow a new RuntimeException as the 
>>>> exception and stack trace will help diagnostics if it does go 
>>>> wrong.  You can get rid of this try-catch block.
>>> OK -- the reason I did this was to insert a readable message into 
>>> the new RuntimeException to indicate the cause of the failure.  I 
>>> think this is good practice and leads to easier diagnosis, but since 
>>> you disagree, I'll take it out.
>>>> L120: is it a leftover debug statement?  I think you meant to add 
>>>> test case to exercise this target handler, right?
>>> removed and a few tests added.
>>> ....Jim
>>>>> I've changed as you've requested, added some additional tests, did 
>>>>> some better error handling in the case of a MemoryHandler not 
>>>>> specifying a target (now throws RuntimeException with an 
>>>>> appropriate message instead of attempting to load a null class and 
>>>>> throwing NPE).  I also corrected the copyrights, tested with JCK, 
>>>>> all jdk_lang tests and have submitted a JPRT job with core tests.
>>>> Great.  Thanks for doing it.
>>>> Mandy
>>>>> I've forwarded a CCC request (separately) and will await its 
>>>>> approval and further review of this change.
>>>>> Thanks,
>>>>>     Jim
>>>>> On 09/28/2012 05:32 PM, Mandy Chung wrote:
>>>>>> On 9/28/2012 12:13 PM, Jim Gish wrote:
>>>>>>> I've re-spun the change with additional usage notes in the spec 
>>>>>>> to reflect the long-standing actual behavior.  Note that it 
>>>>>>> doesn't change the spec per se, as it was already stated in 
>>>>>>> LogManager. This change simply replicates that language with an 
>>>>>>> example in each *Handler class to make it easier to find.
>>>>>> Thanks for looking into it.  This statement in LogManager does
>>>>>> specify the properties for handlers:
>>>>>>   The properties for loggers and Handlers will have names starting
>>>>>>   with the dot-separated name for the handler or logger.
>>>>>> Replicating that statement with an example is one way to do it.
>>>>>> Would it be clearer if the prefix of the properties referenced
>>>>>> in the bullet list is replaced from "java.util.logging" to
>>>>>> some kind of prefix - something like this:
>>>>>>  *<b>Configuration:</b>
>>>>>>  * By default each<tt>ConsoleHandler</tt>  is initialized using 
>>>>>> the following
>>>>>>  *<tt>LogManager</tt>  configuration properties. If properties 
>>>>>> are not defined
>>>>>>  * (or have invalid values) then the specified default values are 
>>>>>> used.
>>>>>>  *<ul>
>>>>>>  *<li> <handler's classname>.level
>>>>>>  *        specifies the default level for the<tt>Handler</tt>
>>>>>>  *        (defaults to<tt>Level.INFO</tt>).
>>>>>>  ...<snip>
>>>>>>  *</ul>
>>>>>>  *
>>>>>>  * For example, the properties for {@code ConsoleHandler} would be:
>>>>>>  *     java.util.logging.ConsoleHandler.level=INFO
>>>>>>  * 
>>>>>> java.util.logging.ConsoleHandler.formatter=java.util.logging.SimpleFormatter
>>>>>>  *
>>>>>>  * For a custom handler, e.g. com.foo.MyHandler, the properties 
>>>>>> would be:
>>>>>>  *     com.foo.MyHandler.level=INFO
>>>>>>  * com.foo.MyHandler.formatter=java.util.logging.SimpleFormatter
>>>>>> This might add some clarity to the spec.
>>>>>> This is a spec bug fix that I would suggest to file a CCC to
>>>>>> track for compatibility.  I would also suggest running the JCK
>>>>>> tests to find out if there is any regression due to this fix.
>>>>>>> The webrev, as posted at 
>>>>>>> http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler/ 
>>>>>> See my comment above w.r.t. the spec change.
>>>>>> test/java/util/logging/MemoryHandler.java
>>>>>>   L27: "via via" typo
>>>>>>   L28: @run tag specifies the test name
>>>>>>        So it should be @run main/othervm MemoryHandler
>>>>>>   L43: jtreg runs the test in a different working directory
>>>>>>   other than the test source.  So the test has to read
>>>>>>   the system property ("test.src") to determine the location
>>>>>>   of the properties file.  Typically, we will do this:
>>>>>>     String src = System.getProperty("test.src", ".);
>>>>>>     File   fname  = new File(src, LM_PROP_FNAME);
>>>>>>   You don't need L44. You can reference LoggingDeadlock3.java test.
>>>>>>   L51: this catch block to throw a RuntimeException doesn't seem
>>>>>>   necessary.  If NPE is thrown, the test will fail anyway.
>>>>>>   One suggestion to the test is to test both cases (one with
>>>>>>   the specified target handler and one without).  You can
>>>>>>   define a custom target handler so that the test can verify
>>>>>>   if the expected one is used.  A simple handler to count
>>>>>>   the number of log messages will do the work.
>>>>>> test/java/util/logging/MemoryHandlerTest.props
>>>>>>   I suggest to take out the comments and just keep the
>>>>>>   properties the test needs to make it easier to tell
>>>>>>   what's configured.
>>>>>>   Perhaps you should also specify
>>>>>>   java.util.logging.MemoryHandler.target to make sure
>>>>>>   that the custom handler with no target handler specified
>>>>>>   will not use j.u.l.MemoryHandler.target as the default.
>>>>>> Mandy
> -- 
> Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
> Oracle Java Platform Group | Core Libraries Team
> 35 Network Drive
> Burlington, MA 01803
> jim.gish at oracle.com

More information about the core-libs-dev mailing list