[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: jmx-dev RFR: 8065213 Specify and implement PlatformMBeanProvider for looking for all platform MB
From:       Daniel Fuchs <daniel.fuchs () oracle ! com>
Date:       2015-01-28 10:44:35
Message-ID: 54C8BD93.8090605 () oracle ! com
[Download RAW message or body]

Hi Shanliang,

This looks good.

ManagementFactory.java:

line 871: there's a stray debug trace that you probably
   forgot to remove:

871             System.out.println("\n\n===== jsl adding: "+provider);


lines 877-885:

  I believe it would improved readability if the content of the
  forEachOrdered statement was factored out in a private static
  method. Something like:

   .forEachOrdered(provider -> addToComponentMap(componentMap, provider))


  private static void addToComponentMap(
                 Map<String,PaltformComponent<?>> cmpMap,
                 PlatformMBeanProvider provider) {
     provider.getPlatformComponentList().stream()
             .collect(toMap(p -> p.getObjectNamePattern(), p -> p,
                            (p1, p2) -> {
                    throw new InternalError(p1.getObjectNamePattern()
                                + " has been used as key for " + p1
                                + ", it cannot be reused for " + p2);
             }))
            .values()
            .stream() // put all components into a map, "putIfAbsent"
            .forEach(p ->
                cmpMap.putIfAbsent(p.getObjectNamePattern(), p));
  }

PlatformMBeanProviderImpl.java:

105          * 3 OperatingSystemMXBean

Not sure what '3' means here - I suggest to remove it.


best regards,

-- daniel


On 27/01/15 18:43, 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.
>
> 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)
>
> Thanks,
> Shanliang
>
>
>

[prev in list] [next in list] [prev in thread] [next in thread]