RFR: 7159567 - inconsistent configuration of MemoryHandler

[Mandy Chung]

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