Re: 8172971: java.management could use System.Logger

[Daniel Fuchs]

Hi Roger,

On 19/01/17 18:22, Roger Riggs wrote:

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,

Well - yes - I don't think it matters much. I didn't want to attempt
to capture all the parameters inside the supplier, and so I simply
changed strb.toString() into strb::toString.

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,

Agreed - but I'd rather not do that now - as this changeset has many
files it would make the changes much more difficult to review.

best regards,

-- daniel

Roger

On 1/19/2017 12:12 PM, Mandy Chung wrote:

On Jan 19, 2017, at 7:30 AM, Daniel Fuchs <daniel.fu...@oracle.com>
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