jmx-dev RFR: 8065213 Specify and implement PlatformMBeanProvider for looking for all platform MBeans

[Mandy Chung]

On 1/27/2015 9:43 AM, shanliang wrote:
> Hi,
>
> Please review:
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8065213
> webrev:   http://cr.openjdk.java.net/~sjiang/JDK-8065213/00/
>
> The class PlatformMBeanProvider.java is added to allow different 
> modules to inform ManagementFactory of their Plaform MBeans. In this 
> version we use the ServiceLoader to load these Plaform MBeans, later 
> we will replace the ServiceProvider by jigsaw mechanisms.
>
Thanks for taking on this one.

This is the first step toward separating com.sun.management API from 
java.management module as Java SE module must only export Java SE API 
per the principles described in JEP 200.   This patch separates the 
standard java.lang.management and JDK-specific MBeans to be provided by 
two different providers.   The next task is to separate the 
com.sun.management API and its implementation from java.management into 
a different module (e.g. jdk.management). This will enable 
java.lang.management to work with and without jdk.management 
(JDK-specific MBeans) when the module system moves further along.

> there are 2 providers in this version:
>    DefaultPlatformMBeanProvider: for all MBeans in the 
> java.lang.management.* (java.management module)
>    com/sun/management/internal/PlatformMBeanProviderImpl: for the 
> MBeans in com.sun.management.* com.sun.management.* (jdk.management 
> module)

This looks right.

The patch looks okay and the ManagementFactory change using service providers
is quite clean.  Review comments:

ManagementFactory.java
    line 760: would be helpful to add some comments describing
    that the implementation of this method.  You can use
    MemoryManagerMXBean and GarageCollectorMXBean as the example.

    Comments for the addProxyNames

    line 867: it would be more readable if you break this into
    two statements.
      List<PlatofrmMBeanProvider> providers = AccessController.doPrivileged(...);
      providers.stream()
               .forEachOrdered(...);
    line 875-886: worths formatting

    line 885 - instead of creating a HashMap and put entry in the forEach call.
    You could have do:
      .collect(toMap(PlatformComponent::getObjectNamePattern, Function.identity())

    You could also have the getMap() be called to initialize the componentMap
    static field (i.e. initialize it in the static initializer rather than lazily
    so the providers must be loaded anyway.

    Can you add comments for the PlatformMBeanFinder methods?

com/sun/management/internal/PlatformMBeanProviderImpl.java
    line 43: does this mxbeanList have to be created lazily?
    Would it be better to make it a final field and create it at the constructor?
   
    line 58-59, 66-67, 111-112, 118-119: nit formatting

java/lang/management/DefaultPlatformMBeanProvider.java
    line 42: should this mxbeanList be a final field?
    you can replace java.lang.management.MemoryManagerMXBean.class
    with MemoryManagerMXBean.class as it's in the same package.
    Same apply to other java.lang.management classes.

    Some formatting nit like mentioned above.

Thanks

Mandy