RFR: 7159567 - inconsistent configuration of MemoryHandler

[Mandy Chung]

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.

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.

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.

L120: is it a leftover debug statement?  I think you meant to add test 
case to exercise this target handler, right?

> 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
>>
>