Thanks for the review Roger, please see my comments inline.
On 8/3/2017 8:23 PM, Roger Riggs wrote:
Hi Ujwal,
(Reviewer, but not specifically servicability).
Comments,
java/lang/management/ThreadMXBean.java:
809: It may be useful to state that the behavior is the same as {@link
#dumpAllThreads} except that the depth is limited.
828: Do not duplicate specification of the meaning of maxDepth =
MAX_VALUE, either put the entire spec
in the @param or in the description. Duplication creates an
opportunity for disagreement or a maintenance issue when updating.
833: terminate the {@code maxDepth} before "is negative".
As Erik comments, there should be a space after "," everywhere; both
to be consistent with existing style and the style guide.
will make changes to doc as suggested.
sun/management/ThreadImpl.java:
484: Since the native code is going to check maxDepth, why is it
checked here also?
-or- remove the check in native
As maxDepth value should not be negative for dumpAllThreads
sun/management/ThreadImpl.java: 484: throws IllegalArgumentException.
Check on native side can be removed but having it might help in
enforcing the convention of using only -1 for dumping all the stack frames
otherwise any negative number can be passed to dump all the frames.
490: This could be simpler to not translate maxDepth to -1,
Here and in the native code, passing MAX_INTEGER would dump the
entire stack, there is no need to special case it
services/threadService.cpp:
565: checking count >= maxDepth is a sufficient test if to dump all
frames, passing MAX_INTEGER is used
and it will dump zero should a negative number get this far.
currently getThreadInfo method accepts maxDepth argument and it uses the
translation of maxDepth to "-1" as it's implementation detail for
printing all stack frames,
As getThreadInfo also uses services/threadService.cpp: 565:
(dump_stack_at_safepoint) expecting it to print all stack trace elements
when maxDepth is given as "-1".
I followed same convention so that it will not affect getThreadInfo
method with maxDepth argument.
Thanks, Roger
Thanks,
Ujwal
On 8/3/2017 10:18 AM, Ujwal Vangapally wrote:
kindly use the below link for accessing webrev
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.01/
previously shared link is no longer accessible hence providing this
new link.
Thanks,
Ujwal.
On 8/3/2017 3:08 PM, Ujwal Vangapally wrote:
Hi,
kindly review the changes made.
https://bugs.openjdk.java.net/browse/JDK-8185003
webrev:
http://cr.openjdk.java.net/~uvangapally/webrev/2017/8185003/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8185705
Thanks,
Ujwal.
