On 06/07/2013 11:07 AM, Daniel Fuchs wrote: >> http://cr.openjdk.java.net/~jbachorik/8010285/webrev.04 > > Hi Jaroslav, > > This looks good to me. > > I assume you've been running both the java.lang & javax.management > unit tests & JCK (java.lang JCK also has some test cases that > indirectly involve JMX introspection).
Yes, I've run all the regtests and JCKs for api/javax_management and api/java_lang. No regressions were detected. > > Also, maybe you could add a test to check that > the system property which allows to revert to the old > behavior is taken into account? I've added the tests for the proper behaviour when the "com.sun.jmx.mbeans.allowNonPublic" system property is set to true. http://cr.openjdk.java.net/~jbachorik/8010285/webrev.05/ -JB- > > good work! > > -- daniel > > > On 6/7/13 9:13 AM, Jaroslav Bachorik wrote: >> 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- >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>> >>>>>> >>>> >>>> >>>> >>> >> >> >
