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