RFR: 7159567 - inconsistent configuration of MemoryHandler

[Jim Gish]

See updated webrev: 
http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler

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