Hi Daniel,
Very straight-forward.
I can see many places where the supplier form of logging could be used
instead
of the if (loggable...) {log(...) }; pattern. But maybe not worth the
conversion effort or not part of this change.
And in some places there is both the if(loggable) and the supplier pattern
which results in a double check of isLoggable, Explicitly calling the
sb.toString(),
instead of using the supplier form would avoid the double check. For
example,
new/src/java.management/share/classes/javax/management/MBeanServerFactory.java:
@@ -504,15 +502,12 @@
throw new JMRuntimeException(msg, x);
}
} catch (RuntimeException x) {
- if (MBEANSERVER_LOGGER.isLoggable(Level.FINEST)) {
+ if (MBEANSERVER_LOGGER.isLoggable(Level.DEBUG)) {
StringBuilder strb = new StringBuilder()
.append("Failed to instantiate MBeanServerBuilder: ").append(x)
.append("\n\t\tCheck the value of the ")
.append(JMX_INITIAL_BUILDER).append(" property.");
- MBEANSERVER_LOGGER.logp(Level.FINEST,
- MBeanServerFactory.class.getName(),
- "checkMBeanServerBuilder",
- strb.toString());
+ MBEANSERVER_LOGGER.log(Level.DEBUG, strb::toString);
There are a few files with very long lines that could be wrapped if it
was convenient.
(To make future reviews easier).
- JmxMBeanServer.java, MLet.java, ModelMBeanAttributeInfo.java,
ModelMBeanConstructorInfo.java,
ModelMBeanNotificationInfo.java, ModelMBeanOperationInfo.java,
RequiredModelMBean.java,
RelationService.java, Timer.java,
Roger
On 1/19/2017 12:12 PM, Mandy Chung wrote:
On Jan 19, 2017, at 7:30 AM, Daniel Fuchs <[email protected]> wrote:
Hi,
Please find below a patch for:
8172971: java.management could use System.Logger
https://bugs.openjdk.java.net/browse/JDK-8172971
webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8172971/webrev.00/
This looks good in general and pretty straight forward change.
Is it intentional to change the level FINEST to DEBUG as opposed to TRACE in a
couple places? For example,
src/java.management/share/classes/javax/management/MBeanServerFactory.java
- if (MBEANSERVER_LOGGER.isLoggable(Level.FINEST)) {
+ if (MBEANSERVER_LOGGER.isLoggable(Level.DEBUG)) {
src/java.management/share/classes/com/sun/jmx/mbeanserver/MBeanServerDelegateImpl.java
- if (MBEANSERVER_LOGGER.isLoggable(Level.FINEST)) {
- MBEANSERVER_LOGGER.logp(Level.FINEST,
- MBeanServerDelegateImpl.class.getName(),
- "getAttributes",
+ if (MBEANSERVER_LOGGER.isLoggable(Level.TRACE)) {
+ MBEANSERVER_LOGGER.log(Level.TRACE,
"Attribute " + attn[i] + " not found");
}
I have also added a new test:
test/sun/management/LoggingTest/LoggingTest.java
This test should have @modules java.logging and java.management.
Mandy
