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

[Daniel Fuchs]

Hi Jaroslav,

On 30/01/15 19:40, Jaroslav Bachorik wrote:
> Hi,
>
> On 30.1.2015 18:38, shanliang wrote:
>> Hi,
>>
>> Thanks for all your comments, here is the new version:
>>     http://cr.openjdk.java.net/~sjiang/JDK-8065213/01/
>
> * java/lang/management/ManagementFactory.java
> L775-788 Can't you use 'return
> getPlatformComponents().stream().flatMap(p->getProxyNames(p, connection,
> mxbeanInterface)).distinct().map(name->newPlatformMXBeanProxy(connection, name,
> mxbeanInterface)).collect(Collectors.toList())' instead of building up a
> new stream via concat?

distinct() is a nice idea - but the crux of the issue is that
getProxyNames throws IOException. We could change it to catch
IOException and throw UncheckedIOException instead, then catch
UncheckedIOException and unwrap it to throw the wrapped IO, but
I believe it would end up being more ugly than what we have today...

cheers,

-- daniel

>
> L846-851 There is no real need to use 'tmp' for validating the object
> name. Just instantiate the object name within the try block starting at
> L854 and adjust the catch handler and you are good to go.
>
> * sun/management/Flag.java
> Changes in this class (making certain methods public) do not relate to
> any other changes in this changeset. Perhaps a mistake?
>
> -JB-
>
>>
>>
>> Mandy Chung wrote:
>>> 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.
>> Done.
>>>
>>>    Comments for the addProxyNames
>> Changed the method name to getProxyNames and added the comments
>>>
>>>    line 867: it would be more readable if you break this into
>>>    two statements.
>>>      List<PlatofrmMBeanProvider> providers =
>>> AccessController.doPrivileged(...);
>>>      providers.stream()
>>>               .forEachOrdered(...);
>> Done
>>>    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())
>> Yes, as:
>>     .collect(toMap(PlatformComponent::getObjectNamePattern,
>> Function.identity(), (p1, p2) -> p1));
>>>
>>>    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.
>> componentMap is now initialized statically.
>>>
>>>    Can you add comments for the PlatformMBeanFinder methods?
>> Yes done
>>>
>>> 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?
>> Done as final
>>>      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?
>> Done
>>>    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.
>> Done
>>
>>>>> 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?
>>>
>>> These providers will need to be loaded and the mxbeanList will be
>>> looked at except for the cases that we are sure that the MXBean only
>>> comes from the default provider.  I see the cost of constructing
>>> mxbeanList involves loading several classes.  If performance is an
>>> issue, perhaps the fast-path would be in ManagementFactory to defer
>>> loading providers and may be better to follow up the performance side
>>> in the second phase (that I expect more changes to sun/management
>>> classes)
>>>
>>> You may want to consider using limited doPrivileged (that can be done
>>> in the second phase).
>> OK, now they are final, I will add doPrivileged in next version.
>>
>>
>>
>>
>> Daniel Fuchs wrote:
>>> 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));
>>>  }
>> You must know that code was changed :)
>>>
>>> PlatformMBeanProviderImpl.java:
>>>
>>> 105          * 3 OperatingSystemMXBean
>>>
>>> Not sure what '3' means here - I suggest to remove it.
>> OK
>>
>> Thanks,
>> Shanliang
>>
>