Re: RFR: 7159567 - inconsistent configuration of MemoryHandler
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:
*bConfiguration:/b
* By default eachttConsoleHandler/tt is initialized using
the following
*ttLogManager/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 thettHandler/tt
*(defaults tottLevel.INFO/tt).
...snip
*/ul
*
* For example, the properties for {@code ConsoleHandler} would be:
* java.util.logging.ConsoleHandler.level=INFO
*
Re: RFR: 7159567 - inconsistent configuration of MemoryHandler
On 10/25/2012 10:25 AM, Jim Gish wrote: OK. One more time. http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler Great, thanks! I'll push it for you. 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. I like a clear error message too. If the test fails, you will need the original exception and the stack trace to diagnose the problem anyway and in some cases, the exception is clear enough. Anyway, just to explain why I had the comment. I'm okay with what you have. Mandy 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: *bConfiguration:/b * By default eachttConsoleHandler/tt is initialized using the following *ttLogManager/tt configuration properties. If properties are not
Re: RFR: 7159567 - inconsistent configuration of MemoryHandler
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:
*bConfiguration:/b
* By default eachttConsoleHandler/tt is initialized using the
following
*ttLogManager/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 thettHandler/tt
*(defaults tottLevel.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
Re: RFR: 7159567 - inconsistent configuration of MemoryHandler
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:
*bConfiguration:/b
* By default eachttConsoleHandler/tt is initialized using the
following
*ttLogManager/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 thettHandler/tt
*(defaults tottLevel.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.
Re: RFR: 7159567 - inconsistent configuration of MemoryHandler
Fixed. Just needed to change a ul to /ul in StreamHandler.java Jim On 10/17/2012 02:46 PM, Jim Gish wrote: I just discovered a small syntax error in StreamHandler (thanks to specdiff :-)). I'll regenerate the webrev shortly. Jim On 10/17/2012 12:41 PM, Jim Gish wrote: Thanks. I believe /li tags are optional, and since they weren't there before, I didn't put them in. The generated javadoc looks ok. Jim On 10/17/2012 11:39 AM, Alan Bateman wrote: On 15/10/2012 20:18, Jim Gish wrote: Here's an updated webrev that fixes some javadoc syntax issues: http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler/ http://cr.openjdk.java.net/%7Ejgish/Bug7159567-set-logging-MemoryHandler/ Thanks, Jim I looked through the javadoc updates and it looks fine to me. One small thing is that it looks like the end-list-item (/li) tags are missing. -Alan -- 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.g...@oracle.com
Re: RFR: 7159567 - inconsistent configuration of MemoryHandler
Here's an updated webrev that fixes some javadoc syntax issues:
http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler/
http://cr.openjdk.java.net/%7Ejgish/Bug7159567-set-logging-MemoryHandler/
Thanks,
Jim
On 10/11/2012 05:37 PM, Jim Gish wrote:
Please review the updated changes at
http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler/
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.
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:
*bConfiguration:/b
* By default eachttConsoleHandler/tt is initialized using the
following
*ttLogManager/tt configuration properties. If properties are
not defined
* (or have invalid values) then the specified default values are used.
*ul
*lihandler's classname.level
*specifies the default level for thettHandler/tt
*(defaults tottLevel.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.g...@oracle.com
Re: RFR: 7159567 - inconsistent configuration of MemoryHandler
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. The webrev, as posted at http://cr.openjdk.java.net/~jgish/Bug7159567-set-logging-MemoryHandler/ http://cr.openjdk.java.net/%7Ejgish/Bug7159567-set-logging-MemoryHandler/ Thanks, Jim On 09/27/2012 10:05 AM, Jim Gish wrote: I agree. I reached the same conclusion, but wanted to see how others reacted. Can we handle the spec change separate from the bug fix? If so, I'll file another spec bug, and proceed with this fix. The key part of the current language that leaves this open and undefined as it is is By default eachttMemoryHandler/tt is initialized using the following LogManager configuration properties. and then refers to java.util.logging.foo properties. Thanks, Jim On 09/26/2012 10:44 PM, Mandy Chung wrote: Hi Jim, On 9/26/2012 2:19 PM, Jim Gish wrote: Please review http://cr.openjdk.java.net/~jgish/Bug7159567-set-MemoryHandler-target/ http://cr.openjdk.java.net/%7Ejgish/Bug7159567-set-MemoryHandler-target/ Overview - currently you can sub-class java.util.logging.MemoryHandler and specify its configuration in a logging.properties file via the classname. For example, if com.foo.MyMemoryHandler extends MemoryHandler, you can have: logging.properties: com.foo.MyMemoryHandler.push=WARNING com.foo.MyMemoryHandler.level=FINEST The current implementation does use thehandler-classname.* properties. However I couldn't find it from the j.u.logging specification. Did I miss any javadoc that mentions this? Per the j.u.l.MemoryHandler specification, it only reads java.util.logging.MemoryHandler.* properties but not the properties with the subclass name. *bConfiguration:/b * By default eachttMemoryHandler/tt is initialized using the following * LogManager configuration properties. If properties are not defined * (or have invalid values) then the specified default values are used. * If no default value is defined then a RuntimeException is thrown. *ul *lijava.util.logging.MemoryHandler.level *specifies the level for thettHandler/tt *(defaults tottLevel.ALL/tt). *lijava.util.logging.MemoryHandler.filter *specifies the name of attFilter/tt class to use *(defaults to nottFilter/tt). *lijava.util.logging.MemoryHandler.size *defines the buffer size (defaults to 1000). *lijava.util.logging.MemoryHandler.push *defines thettpushLevel/tt (defaults tottlevel.SEVERE/tt). *lijava.util.logging.MemoryHandler.target *specifies the name of the targetttHandler/tt class. *(no default). */ul I looked at the change history and found that the change to read property using the classname as the prefix (rather than j.u.l.MemoryHandler) was done in JDK 5 in the fix for 4635817. This isn't related to the bug you're fixing but I think this deserves to investigate whether this was an intended spec change at that time; if so, a spec update to clarify this behavior would be good. Thanks 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.g...@oracle.com
Re: RFR: 7159567 - inconsistent configuration of MemoryHandler
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:
*bConfiguration:/b
* By default eachttConsoleHandler/tt is initialized using the following
*ttLogManager/tt configuration properties. If properties are not defined
* (or have invalid values) then the specified default values are used.
*ul
*lihandler's classname.level
*specifies the default level for thettHandler/tt
*(defaults tottLevel.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
Re: RFR: 7159567 - inconsistent configuration of MemoryHandler
Sorry -- wrong list -- I mean to send to the openjdk list. On 09/26/2012 05:19 PM, Jim Gish wrote: Please review http://cr.openjdk.java.net/~jgish/Bug7159567-set-MemoryHandler-target/ http://cr.openjdk.java.net/%7Ejgish/Bug7159567-set-MemoryHandler-target/ Overview - currently you can sub-class java.util.logging.MemoryHandler and specify its configuration in a logging.properties file via the classname. For example, if com.foo.MyMemoryHandler extends MemoryHandler, you can have: logging.properties: com.foo.MyMemoryHandler.push=WARNING com.foo.MyMemoryHandler.level=FINEST etc. However, the only way to specify target is by using the global property, java.util.logging.MemoryHandler.target. For example, java.util.logging.MemoryHandler.target=java.util.logging.ConsoleHandler This is clearly broken. This bug fix allows you to say: com.foo.MyMemoryHandler.target=java.util.logging.ConsoleHandler Thanks, Jim -- 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.g...@oracle.com
Re: RFR: 7159567 - inconsistent configuration of MemoryHandler
Time to go home :-( I did send to the right list-- it's just that my mailer didn't show the whole address. Yikes! Sorry about the spam. Please review as originally requested. On 09/26/2012 06:21 PM, Jim Gish wrote: Sorry -- wrong list -- I mean to send to the openjdk list. On 09/26/2012 05:19 PM, Jim Gish wrote: Please review http://cr.openjdk.java.net/~jgish/Bug7159567-set-MemoryHandler-target/ http://cr.openjdk.java.net/%7Ejgish/Bug7159567-set-MemoryHandler-target/ Overview - currently you can sub-class java.util.logging.MemoryHandler and specify its configuration in a logging.properties file via the classname. For example, if com.foo.MyMemoryHandler extends MemoryHandler, you can have: logging.properties: com.foo.MyMemoryHandler.push=WARNING com.foo.MyMemoryHandler.level=FINEST etc. However, the only way to specify target is by using the global property, java.util.logging.MemoryHandler.target. For example, java.util.logging.MemoryHandler.target=java.util.logging.ConsoleHandler This is clearly broken. This bug fix allows you to say: com.foo.MyMemoryHandler.target=java.util.logging.ConsoleHandler Thanks, Jim -- 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.g...@oracle.com -- 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.g...@oracle.com
Re: RFR: 7159567 - inconsistent configuration of MemoryHandler
Hi Jim, On 9/26/2012 2:19 PM, Jim Gish wrote: Please review http://cr.openjdk.java.net/~jgish/Bug7159567-set-MemoryHandler-target/ http://cr.openjdk.java.net/%7Ejgish/Bug7159567-set-MemoryHandler-target/ Overview - currently you can sub-class java.util.logging.MemoryHandler and specify its configuration in a logging.properties file via the classname. For example, if com.foo.MyMemoryHandler extends MemoryHandler, you can have: logging.properties: com.foo.MyMemoryHandler.push=WARNING com.foo.MyMemoryHandler.level=FINEST The current implementation does use thehandler-classname.* properties. However I couldn't find it from the j.u.logging specification. Did I miss any javadoc that mentions this? Per the j.u.l.MemoryHandler specification, it only reads java.util.logging.MemoryHandler.* properties but not the properties with the subclass name. *bConfiguration:/b * By default eachttMemoryHandler/tt is initialized using the following * LogManager configuration properties. If properties are not defined * (or have invalid values) then the specified default values are used. * If no default value is defined then a RuntimeException is thrown. *ul *lijava.util.logging.MemoryHandler.level *specifies the level for thettHandler/tt *(defaults tottLevel.ALL/tt). *lijava.util.logging.MemoryHandler.filter *specifies the name of attFilter/tt class to use *(defaults to nottFilter/tt). *lijava.util.logging.MemoryHandler.size *defines the buffer size (defaults to 1000). *lijava.util.logging.MemoryHandler.push *defines thettpushLevel/tt (defaults tottlevel.SEVERE/tt). *lijava.util.logging.MemoryHandler.target *specifies the name of the targetttHandler/tt class. *(no default). */ul I looked at the change history and found that the change to read property using the classname as the prefix (rather than j.u.l.MemoryHandler) was done in JDK 5 in the fix for 4635817. This isn't related to the bug you're fixing but I think this deserves to investigate whether this was an intended spec change at that time; if so, a spec update to clarify this behavior would be good. Thanks Mandy
