RFR: JDK-8185003 JMX: Add a version of ThreadMXBean.dumpAllThreads with a maxDepth argument

[Ujwal Vangapally]

Thanks Daniel,

will make that change in next webrev.

-Ujwal


On 8/11/2017 7:34 PM, Daniel Fuchs wrote:
> Hi Ujwal,
>
> The java part looks good to me.
>
> ThreadMXBean.java:
>
>  775      * @implSpec if not implemented, the method will throw
>  776      * an UnsupportedOperationException.
>
> and
>
>  862      * @implSpec if not implemented, the method will throw
>  863      * an UnsupportedOperationException.
>
> I wonder if a better wording wouldn't be:
>
>   * @implSpec If not overridden in subclasses, the default
>   *           implementation of this method
>   *           throws an UnsupportedOperationException.
>
> best regards,
>
> -- daniel
>
> On 11/08/2017 14:20, Ujwal Vangapally wrote:
>> Gentle Reminder.
>>
>>
>> On 8/9/2017 10:45 PM, Ujwal Vangapally wrote:
>>>
>>> Thanks for the review Mandy,
>>>
>>> kindly see my comments inline.
>>>
>>> webrev: 
>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.03/
>>>
>>> csr: https://bugs.openjdk.java.net/browse/JDK-8185705
>>>
>>> Ujwal
>>>
>>>
>>> On 8/9/2017 5:23 AM, mandy chung wrote:
>>>>
>>>>
>>>>
>>>> On 8/8/17 1:27 AM, Ujwal Vangapally wrote:
>>>>> Hi,
>>>>>
>>>>> below is the link to new webrev incorporating review comments.
>>>>>
>>>>> webrev: 
>>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.02/ 
>>>>>
>>>>>
>>>> 769 * The Behaviour is same as {@link #getThreadInfo(long[], 
>>>> boolean, boolean)}
>>>> 770 * except that the stack trace depth is limited.
>>>> 771 *
>>>> Alternatively you can copy the javadoc from getThreadInfo(long[], 
>>>> boolean, boolean) to the new method, saying:
>>>>
>>>> Returns the thread info for each thread whose ID is in the input 
>>>> array {@code ids}, with stack trace of the specified maxinum number 
>>>> of elements and synchronization information.
>>>>
>>>> getThreadInfo(long[], boolean, boolean) can replace most of the 
>>>> javadoc by saying:  This is equivalent to calling:
>>>> getThreadInfo(ids, lockedMonitors, lockedSynchronizers, 
>>>> Integer.MAX_VALUE)
>>>>
>>>> Same applies to dumpAllThreads.
>>>>
>>> did it as you suggested but it is not shown very clearly in webrev, 
>>> kindly take a look.
>>>> I did look at grepcode and see any custom implementation of 
>>>> ThreadMXBean.  There are cases that extends ThreadMXBean which 
>>>> mostly attempts to call com.sun.management.ThreadMXBean without 
>>>> referencing the type.  They are not custom implementation of 
>>>> ThreadMXBean.
>>>>
>>>> I agree with Daniel's suggestion to make the new methods with a 
>>>> default implementation throwing UOE.  This might make existing 
>>>> implementation easier to migrate.
>>>>
>>> made them default methods.
>>>> sun/management/ThreadImpl.java
>>>> 495 if(maxDepth < 0) { 500 return dumpThreads0(null, 
>>>> lockedMonitors, lockedSynchronizers,maxDepth);
>>>> missing space between if and ( and ,
>>>>
>>> changed it.
>>>> src/java.management/share/native/libmanagement/ThreadImpl.c
>>>>    long line 138.  You can wrap it.
>>>>
>>> did it.
>>>> src/share/vm/services/jmm.h
>>>>    This changes the interface and so you should add JMM_VERSION_10 
>>>> even we don't support mixing hotspot with of different version of 
>>>> JDK libraries.
>>>>
>>> not sure if I made this change correctly please confirm by seeing 
>>> webrev.
>>>> src/share/vm/services/management.cpp
>>>>    line 1163 is quite long line.  Can you wrap it?
>>>>
>>> did it.
>>>>    jmm_DumpThreads - it should probably validate maxDepth as 
>>>> jmm_GetThreadInfo does.
>>> we can do it as an additional check, I didn't find anything which 
>>> calls jmm_DumpThreads with negative value for maxDepth.
>>> As we are verifying maxDepth value at java level and I didn't use -1 
>>> as a special case for maxDepth to print full stack.
>>> But I see no harm in adding an additional check at native level.
>>>
>>>>> verified that
>>>>>
>>>>> MBeanServerConnection.invoke throws ReflectionException when 
>>>>> invoked with a method that doesn't exist in remote MBean server.
>>>>>
>>>>> UndeclaredThrowableException will be throwed when a client gets 
>>>>> proxy of remote MBean server and calls a method that doesn't exist 
>>>>> on remote Mbean server.
>>>>>
>>>>> do we need to develop Automated tests for verifying above cases ?
>>>>>
>>>> It'd be useful unless there is existing JMX tests covering this 
>>>> scenario.  This is more on JMX framework and  it's okay to separate 
>>>> a new issue for adding these new tests.
>>>>
>>>> We should re-read the specification and see if spec clarification 
>>>> is needed.
>>> will do this soon.
>>>>
>>>> Mandy
>>>
>>
>