RFR: 7159567 - inconsistent configuration of MemoryHandler
Jim Gish
jim.gish at oracle.com
Thu Oct 25 17:25:34 UTC 2012
OK. One more time.
http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler
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.
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