8172971: java.management could use System.Logger

[Roger Riggs]

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 <daniel.fuchs at 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