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
