Thanks for the review Mandy,

kindly see my comments inline.




On 8/9/2017 5:23 AM, mandy chung wrote:

On 8/8/17 1:27 AM, Ujwal Vangapally wrote:

below is the link to new webrev incorporating review comments.


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 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.
495 if(maxDepth < 0) { 500 return dumpThreads0(null, lockedMonitors, lockedSynchronizers,maxDepth);
missing space between if and ( and ,

changed it.
   long line 138.  You can wrap it.

did it.
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.
   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.


Reply via email to