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.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
>