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

[Jaroslav Bachorik]

On 30.1.2015 20:41, Daniel Fuchs wrote:
> On 30/01/15 20:05, Jaroslav Bachorik wrote:
>> On 30.1.2015 19:52, Daniel Fuchs wrote:
>>> 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...
>>
>> IDK, since getProxyNames() is used only in this particular method,
>> making it throw UncheckedIOException would mean two additional try/catch
>> blocks. And we wouldn't mess with the evaluation optimizations by
>> creating the intermediary set.
>
> I forgot to mention that newMXBeanProxy also throws an IOException ;-(
> I wouldn't be too much concerned about performances here.
> Creation of an intermediary set containing a few names is nothing
> compared to calling MBeanServerConnection.queryNames.
> Also it doesn't look like something that could be on the critical path.
> If we can avoid manipulating exceptions it's IMO much better.

Ok, I see. With this method being public API it's getting trickier. I 
still don't like the functional/imperative mix but, as you say, probably 
less messy then dealing with checked exceptions.

-JB-

>
> best regards,
>
> -- daniel
>
>>
>> -JB-
>>
>>>
>>> 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
>>>>>
>>>>
>>>
>>
>