jmx-dev RFR: 8010285 Enforce the requirement of Management Interfaces being public

[Jaroslav Bachorik]

On Thu 06 Jun 2013 06:06:52 PM CEST, shanliang wrote:
> Jaroslav Bachorik wrote:
>> On Thu 06 Jun 2013 05:22:31 PM CEST, shanliang wrote:
>>
>>> Jaroslav,
>>>
>>> It is now OK for me about the MBean interface searching in the
>>> Introspector.
>>>
>>> Here is my comment on JMX.java:
>>> 206 -- 212 you added a call
>>>     Introspector.testComplianceMBeanInterface(interfaceClass);
>>>
>>> It is better to move this call to:
>>>     MBeanServerInvocationHandler.newProxyInstance
>>> because the real job is done in newProxyInstance and it could be
>>> directly called by anyone.
>>>
>>
>> Hm, wouldn't it be better to move the actual logic from
>> MBeanServerInvocationHandler.newProxyInstance to JMX.newMBeanProxy and
>> delegate the MBSIH.newProxyInstance back to JMX.newMBeanProxy ? This
>> way it would be consistent with the way JMX.newMXBeanProxy is written.
>>
> I have no opinion here, this is an implementation detail, anyway we
> have to keep both JMX.newMBeanProxy and
> MBeanServerInvocationHandler.newProxyInstance

http://cr.openjdk.java.net/~jbachorik/8010285/webrev.04

I've moved the newMBeanProxy() logic to JMX to keep it consistent  with 
JMX.newMXBeanProxy(); MBSIH.newProxyInstance() is just forwarded to 
JMX.newMBeanProxy()

-JB-

>
> Shanliang
>>
>>> All others are OK for me.
>>>
>>
>> Thanks.
>>
>> -JB-
>>
>>
>>> Shanliang
>>>
>>> Jaroslav Bachorik wrote:
>>>
>>>> On Wed 05 Jun 2013 07:54:10 PM CEST, shanliang wrote:
>>>>
>>>>
>>>>> Daniel Fuchs wrote:
>>>>>
>>>>>
>>>>>> On 6/5/13 3:55 PM, Jaroslav Bachorik wrote:
>>>>>>
>>>>>>
>>>>>>>> class A extends B { ...}
>>>>>>>>
>>>>>>>>
>>>>>>>>> class B implements AMBean {...}
>>>>>>>>>
>>>>>>>>>
>>>>>>> Yes, I see it now. However, when you check the JMX specification, page
>>>>>>> 50 onwards, the current implementation does not seem to be correct.
>>>>>>>
>>>>>>> "3. If MyClass is an instance of the DynamicMBean interface, then
>>>>>>> MyClassMBean is
>>>>>>> ignored. If MyClassMBean is not a public interface, it is not a JMX
>>>>>>> manageable
>>>>>>> resource. If the MBean is an instance of neither MyClassMBean nor
>>>>>>> DynamicMBean, the inheritance tree of MyClass is examined, looking
>>>>>>> for the
>>>>>>> nearest superclass that implements its own MBean interface.
>>>>>>> a. If there is an ancestor called SuperClass that is an instance of
>>>>>>> SuperClassMBean, the design patterns are used to derive the
>>>>>>> attributes and
>>>>>>> operations from SuperClassMBean. In this case, the MBean MyClass then
>>>>>>> has the same management interface as the MBean SuperClass. If
>>>>>>> SuperClassMBean is not a public interface, it is not a JMX manageable
>>>>>>> resource.
>>>>>>> b. When there is no superclass with its own MBean interface, MyClass is
>>>>>>> not a
>>>>>>> Standard MBean."
>>>>>>>
>>>>>>> According to the specification the correct MBean interface for
>>>>>>>
>>>>>>>    class A extends B { ...}
>>>>>>>    class B implements BMBean, AMBean
>>>>>>>
>>>>>>> would be BMBean
>>>>>>>
>>>>>>>
>>>>>> Hi Jaroslav,
>>>>>>
>>>>>> Given that A is an instance of AMBean I think that according to the
>>>>>> specification the correct interface should be AMBean.
>>>>>> It's true that the JMX Specification does not explicitly speak of this
>>>>>> case - but neither does it forbid it.
>>>>>>
>>>>>> My advice would therefore be to clarify the spec on this point,
>>>>>> if that's needed - rather than risking the introduction of
>>>>>> incompatibilities.
>>>>>>
>>>>>> -- daniel
>>>>>>
>>>>>>
>>>>> Look at the spec 1.4:
>>>>> ------
>>>>> 2. If the MyClass  MBean is an instance of a MyClassMBean interface,
>>>>> then only the methods listed in, or inherited by, the interface are
>>>>> considered among all the methods of, or inherited by, the MBean. The
>>>>> design patterns are then used to identify the attributes and
>>>>> operations from the method names in the MyClassMBean interface and its
>>>>> ancestors. In other words, MyClass is a standard MBean
>>>>> ------
>>>>>
>>>>> Here A is an instance of AMBean, according to 2), A is a standard
>>>>> MBean and  AMBean must be taken,
>>>>>
>>>>> 3) specifies the condition as "If the MBean is an instance of neither
>>>>> MyClassMBean nor DynamicMBean", our example is out of this condition,
>>>>> so should not apply 3) to our example.
>>>>>
>>>>>
>>>> Ok. I've reverted to the original implementation.
>>>>
>>>> http://cr.openjdk.java.net/~jbachorik/8010285/webrev.03/
>>>>
>>>> The whole MBean interface inferring algorithm is a bit of black magic,
>>>> though. I mean, checking all the interfaces implemented by all the
>>>> classes in the inheritance hierarchy is counter-intuitive. I mean, why
>>>> would you do something like :
>>>>   MyServiceIfc extends ObscureIfc extends ServiceMBean
>>>>   Service implements MyServiceIfc
>>>>
>>>> and silently supposing that somewhere in the interface inheritance
>>>> hierarchy just happen to be a properly named interface and my Service
>>>> would become a managed resource. Not mentioning the fact, that the
>>>> current implementation will fail to resolve the ServiceMBean as the
>>>> MBean interface - it stops checking by ObscureIfc.
>>>>
>>>> It would be much easier for the user if he just specified the MBean
>>>> interface alongside the implementation class (... implements ...,
>>>> ServiceMBean) cleanly indicating that an object is a managed resource.
>>>>
>>>> But, what you gonna do ... changing the spec would probably open  a
>>>> whole another can of worms and since nobody is complaining about the
>>>> current implementation we can just leave it as it is.
>>>>
>>>> -JB-
>>>>
>>>>
>>>>
>>>>> Shanliang
>>>>>
>>>>>
>>>>>
>>>>>>> and for
>>>>>>>    class A extends B { ...}
>>>>>>>    class B implements AMBean {...}
>>>>>>>
>>>>>>> is not defined; neither B or A are manageable resources.
>>>>>>>
>>>>>>> As I said, the jtreg and jck test does not seem to mind which
>>>>>>> implementation is used, they all pass happily. I would prefer bringing
>>>>>>> the implementation in sync with the specification.
>>>>>>>
>>>>>>> -JB-
>>>>>>>
>>>>>>>
>>>>>>>
>>>>
>>>>
>>
>>
>>
>