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
