On 2/20/18 9:48 PM, Jeremy Manson wrote:
I think that's a much better approach (I didn't notice the validate
method :) )
I think you may want to grab my test changes, or make some similar
ones. Presumably, the tests do not pass with your change.
I ran your test and it passed before posting it.
I think the API wording change is very slightly confusing.
* A {@code CompositeData} does not contain this attribute
* when representing a {@code ThreadInfo} of JDK 6 or
older version.
* This attribute will be set to {@link
Thread#NORM_PRIORITY}.</td>
I'd probably say "In such cases" at the beginning of the second
sentence. Also, is there a rule about when you say JDK 6 and when you
say Java 6?
Java SE 6 should be the proper terminology be used in this case. I
actually try to see if we can use @since instead. I will take any pass
on the spec and how it can tie with @since in the getter methods.
I will look at it later today.
Mandy
Jeremy
On Tue, Feb 20, 2018 at 6:41 PM, mandy chung <mandy.ch...@oracle.com
<mailto:mandy.ch...@oracle.com>> wrote:
Hi Jeremy,
Another approach is to change
ThreadInfoCompositeData::validateCompositeData
to validate the given CompositeData. I also revised
ThreadInfoCompositeData to
return the default value of the attributes, if not present.
This is the patch:
http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/webrev.00/
<http://cr.openjdk.java.net/%7Emchung/jdk11/webrevs/8198253/webrev.00/>
What do you think?
Mandy
On 2/20/18 4:46 PM, Jeremy Manson wrote:
(I dropped serviceability-dev from this thread by mistake! Oops!)
Okay. Here's the revised patch. LMK if that's what you had in mind.
http://cr.openjdk.java.net/~jmanson/8198253/webrev.01/
<http://cr.openjdk.java.net/%7Ejmanson/8198253/webrev.01/>
On Fri, Feb 16, 2018 at 6:20 PM, mandy chung
<mandy.ch...@oracle.com <mailto:mandy.ch...@oracle.com>> wrote:
Hi Jeremy,
lockedMonitors and lockedSynchronizers attribute are not optional if
that's
the issue you try to resolve. I think the specification should be
clarified.
ThreadInfo::from supports the three different versions for
interoperability:
1. CompositeData for JDK 1.5 ThreadInfo with no lockedMonitors and
lockedSynchronizers attribute
2. CompositeData for JDK 6 ThreadInfo with lockedMonitors and
lockedSynchronizers
attributes
3. CompositeData for JDK 9 ThreadInfo with lockedMonitors and
lockedSynchronizers
attributes and with daemon and priority attribute.
JMX client can connect to a running VM in any version and get back a
proper ThreadInfo.
If ThreadInfo::from is called with a CompositeData containing daemon and
priority attribute but lockedMonitors and lockedSynchronizers
attributes are
absent then the given CompositeData is invalid and
IllegalArgumentException
should be thrown.
Does this help?
Mandy
On 2/15/18 4:46 PM, Jeremy Manson wrote:
Hi folks,
Been a long time! I'd like someone to do a code review.
This is in code I wrote a few years ago, and got wrong. At
the time, David Holmes, Staffan Larsen, and Mandy Chung
reviewed it. It does mean that people using
ThreadInfo.from(CompositeData) may be getting the wrong
values out for ThreadInfo, so it is definitely worth fixing.
The patch below fixes the bug, but is a pretty questionable
approach. Better approaches happily considered.
Patch:
http://cr.openjdk.java.net/~jmanson/8198253/webrev.00/
<http://cr.openjdk.java.net/%7Ejmanson/8198253/webrev.00/>
Bug:
https://bugs.openjdk.java.net/browse/JDK-8198253
<https://bugs.openjdk.java.net/browse/JDK-8198253>
Thanks!
Jeremy
