On Tue 18 Jun 2013 12:25:35 PM CEST, Daniel Fuchs wrote:
> Hi Jaroslav,
>
> > 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/
>
> Thanks for adding the test. I haven't looked at the source again,
> I trust nothing changed there. Some issues in the tests:
>
> MBeanFallbackTest.java:
>
> 57 if (failures > 0) {
> 58 System.exit(1);
> 59 }
>
> I think
> throw new Exception("TEST FAILURES: " + failures);
> would be more appropriate.
I saw both of them used across the regtests and was not really sure
which is the recommended way of signalling the test failure ...
>
> Also I see that you have an @run main line - I believe it
> should be @run main/othervm since the test sets some
> system properties.
Currently all the testst in javax/management are run as main/othervm.
But it won't hurt to make the intention of running the test in a
separate JVM visually clear.
The same applies to the rest of the new tests.
>
>
> MXBeanFallbackTest.java:
>
> 30 * @author Eamonn McManus
Yep :/
-JB-
>
> Is that a copy paste issue?
>
> 47 System.in.read();
>
> Left over from debugging session - right ;-) ?
>
> I see that you have an @run main line there too - I
> believe it should be @run main/othervm since this
> test also sets some system properties.
>
>
> JMXProxyFallbackTest.java:
>
> 63 if (failures > 0) {
> 64 System.exit(1);
> 65 }
>
> should throw new Exception("TEST FAILURES: " + failures);
> instead.
>
> I see that you have an @run main line there too - I
> believe it should be @run main/othervm since this
> test also sets some system properties.
>
> JMXProxyTest.java:
>
> 120 if (failures > 0) {
> 121 System.exit(1);
> 122 }
>
> should throw new Exception("TEST FAILURES: " + failures);
> instead.
>
> best regards,
>
> -- daniel
>
>
> On 6/18/13 12:01 PM, Jaroslav Bachorik wrote:
>> 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-
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>
