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

[Ujwal Vangapally]

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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20170811/23e9c8a9/attachment.html>