[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR - [RFE] JDK-8007141 : Introduce Dynamic MBean exposing the perf counters
From: Harsha Wardhana B <harsha.wardhana.b () oracle ! com>
Date: 2017-03-08 4:24:12
Message-ID: b65dcffe-4bb7-10e8-f8dc-f1c45fa2a95f () oracle ! com
[Download RAW message or body]
Hi Daniel,
On Tuesday 07 March 2017 08:40 PM, Daniel Fuchs wrote:
> Hi Harsha,
>
> Not a review either (at least not a complete one).
>
> PlatformMBeanProviderImpl:
>
> 281 @Override
> 282 public Set<Class<? extends DynamicMBean>>
> mbeanInterfaces() {
> 283 return Collections.emptySet();
> 284 }
>
> For future maintainers, I think it would be good to copy
> the comment at line 250 and insert it before line 283 too:
> 250 // DynamicMBean cannot be used to find an
> MBean by ManagementFactory
>
Will do.
> HotSpotPerfCounterMBean:
>
> Exception handling: I have the feeling that this part might
> be a bit over-engineered. If you look at javax.management.StandardMBean
> (which is a canonical implementation of DynamicMBean) and
> then transitively at MBeanSupport and PerInterface where this class
> eventually delegates to, you will see that:
>
> 1. You can throw NPE in setAttribute when attribute == null (no need for
> all the wrapping)
If attribute == null, isn't it better to throw IllegalArgumentException
instead of NPE? I'll remove the wrapping.
> 2. You should throw AttributeNotFoundException when attempting to read
> a write-only attribute or write a read-only attribute
Isn't UnSupportedOperationEx better than AttributeNotFoundException?
Have we committed to above in the specs?
> 3. You can throw NPE if AttributeList is null
If attribute == null, isn't it better to throw IllegalArgumentException
instead of NPE? I'll remove the wrapping.
>
> 179 String typeName = "java.lang.String";
>
> That looks like a hack.
If attribute is null, defaulting to String would be convenient as
getAttribute can return empty string instead of null. Do you think
otherwise?
>
> What if at some point the value of that attribute becomes non null,
> and its class is *not* java.lang.String?
> Then MBeanAttributeInfo would be lying.
> If that ever happens - then it would be more consistent to coerce
> the value to its declared type (String) before sending it as a
> a result for getAttribute.
Since perf-counters are key-value pairs written into shared memory, it
is reasonable to expect new counters to be added or type of existing
counters to be updated while VM is running. I have to refresh the
counters periodically (maybe once every 5 mins) since I will not be
notified of any change. If any attributes are added or if existing
attribute types are changed, then AttributeChangeNotification will be
sent. Clients will have to refresh MBeanInfo upon receiving this
notification. So yeah, there is a brief window where MBeanInfo will be
lying. I don't see easy way to fix it unlesss perfcounters can notify
MBean of any mutations. Maybe the limitation can be documented.
I will make these changes in next webrev.
> Or better, find a way to figure out what should be the class name
> of the value even if the value is null (isn't there some metadata
> available for these perf counters somewhere?).
>
> PerfCounterMBeanTest:
>
> testGetAttribute should verify that class of the returned value
> corresponds to what was declared in the MBeanAttributeInfo for the
> corresponding attribute.
Sure. Will do.
>
> cheers,
>
> -- daniel
-Harsha
>
> On 26/02/17 16:50, Harsha Wardhana B wrote:
>> Hi All,
>>
>> Please review the below RFE,
>>
>> JDK-8007141 <http://JDK-8007141> : Introduce Dynamic MBean exposing the
>> perf counters.
>>
>> with webrev at,
>>
>> http://cr.openjdk.java.net/~hb/8007141/webrev.00/
>>
>> Appreciate if I can get inputs for below.
>>
>> 1. Location of HotSpotPerfCounterMBean. It is located at
>> src/jdk.management/share/classes/com/sun/management/internal. It can be
>> moved to src/jdk.management/share/classes/com/sun/management if
>> required.
>>
>> 2. Javadoc comments for HotSpotPerfCounterMBean. Not sure if it is
>> adequate.
>>
>> 3. Description for each attribute of MBean. I am using getUnits(),
>> getVariability(), and getFlags() for description. I am not sure if that
>> is the right description. Appreciate inputs from someone who knows perf
>> counters well.
>>
>> Thanks
>>
>> Harsha
>>
>>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>Hi Daniel,</p>
<br>
<div class="moz-cite-prefix">On Tuesday 07 March 2017 08:40 PM,
Daniel Fuchs wrote:<br>
</div>
<blockquote
cite="mid:a60e71b7-c5f5-2009-a9a3-4c7bc0b92908@oracle.com"
type="cite">Hi Harsha, <br>
<br>
Not a review either (at least not a complete one). <br>
<br>
PlatformMBeanProviderImpl: <br>
<br>
281 @Override <br>
282 public Set<Class<? extends
DynamicMBean>> mbeanInterfaces() { <br>
283 return Collections.emptySet(); <br>
284 } <br>
<br>
For future maintainers, I think it would be good to copy <br>
the comment at line 250 and insert it before line 283 too: <br>
250 // DynamicMBean cannot be used to \
find an MBean by ManagementFactory <br>
<br>
</blockquote>
Will do.<br>
<blockquote
cite="mid:a60e71b7-c5f5-2009-a9a3-4c7bc0b92908@oracle.com"
type="cite">HotSpotPerfCounterMBean: <br>
<br>
Exception handling: I have the feeling that this part might <br>
be a bit over-engineered. If you look at
javax.management.StandardMBean <br>
(which is a canonical implementation of DynamicMBean) and <br>
then transitively at MBeanSupport and PerInterface where this
class <br>
eventually delegates to, you will see that: <br>
<br>
1. You can throw NPE in setAttribute when attribute == null (no
need for <br>
all the wrapping) <br>
</blockquote>
If attribute == null, isn't it better to throw
IllegalArgumentException instead of NPE? I'll remove the wrapping. <br>
<blockquote
cite="mid:a60e71b7-c5f5-2009-a9a3-4c7bc0b92908@oracle.com"
type="cite"> 2. You should throw AttributeNotFoundException when
attempting to read <br>
a write-only attribute or write a read-only attribute <br>
</blockquote>
Isn't UnSupportedOperationEx better than AttributeNotFoundException?
Have we committed to above in the specs?<br>
<blockquote
cite="mid:a60e71b7-c5f5-2009-a9a3-4c7bc0b92908@oracle.com"
type="cite"> 3. You can throw NPE if AttributeList is null <br>
</blockquote>
If attribute == null, isn't it better to throw
IllegalArgumentException instead of NPE? I'll remove the wrapping.
<blockquote
cite="mid:a60e71b7-c5f5-2009-a9a3-4c7bc0b92908@oracle.com"
type="cite"> <br>
179 String typeName = "java.lang.String"; <br>
<br>
That looks like a hack. <br>
</blockquote>
If attribute is null, defaulting to String would be convenient as
getAttribute can return empty string instead of null. Do you think
otherwise?<br>
<blockquote
cite="mid:a60e71b7-c5f5-2009-a9a3-4c7bc0b92908@oracle.com"
type="cite"> <br>
What if at some point the value of that attribute becomes non
null, <br>
and its class is *not* java.lang.String? <br>
</blockquote>
<blockquote
cite="mid:a60e71b7-c5f5-2009-a9a3-4c7bc0b92908@oracle.com"
type="cite">Then MBeanAttributeInfo would be lying. <br>
</blockquote>
<blockquote
cite="mid:a60e71b7-c5f5-2009-a9a3-4c7bc0b92908@oracle.com"
type="cite">If that ever happens - then it would be more
consistent to coerce <br>
the value to its declared type (String) before sending it as a <br>
a result for getAttribute. <br>
</blockquote>
Since perf-counters are key-value pairs written into shared memory,
it is reasonable to expect new counters to be added or type of
existing counters to be updated while VM is running. I have to
refresh the counters periodically (maybe once every 5 mins) since I
will not be notified of any change. If any attributes are added or
if existing attribute types are changed, then
AttributeChangeNotification will be sent. Clients will have to
refresh MBeanInfo upon receiving this notification. So yeah, there
is a brief window where MBeanInfo will be lying. I don't see easy
way to fix it unlesss perfcounters can notify MBean of any
mutations. Maybe the limitation can be documented. <br>
<br>
I will make these changes in next webrev. <br>
<br>
<blockquote
cite="mid:a60e71b7-c5f5-2009-a9a3-4c7bc0b92908@oracle.com"
type="cite">Or better, find a way to figure out what should be the
class name <br>
of the value even if the value is null (isn't there some metadata
<br>
available for these perf counters somewhere?). <br>
</blockquote>
<blockquote
cite="mid:a60e71b7-c5f5-2009-a9a3-4c7bc0b92908@oracle.com"
type="cite"> <br>
PerfCounterMBeanTest: <br>
<br>
testGetAttribute should verify that class of the returned value <br>
corresponds to what was declared in the MBeanAttributeInfo for the
<br>
corresponding attribute. <br>
</blockquote>
Sure. Will do. <br>
<blockquote
cite="mid:a60e71b7-c5f5-2009-a9a3-4c7bc0b92908@oracle.com"
type="cite"> <br>
cheers, <br>
<br>
-- daniel <br>
</blockquote>
-Harsha<br>
<blockquote
cite="mid:a60e71b7-c5f5-2009-a9a3-4c7bc0b92908@oracle.com"
type="cite"> <br>
On 26/02/17 16:50, Harsha Wardhana B wrote: <br>
<blockquote type="cite">Hi All, <br>
<br>
Please review the below RFE, <br>
<br>
JDK-8007141 <a class="moz-txt-link-rfc2396E"
href="http://JDK-8007141"><http://JDK-8007141></a> :
Introduce Dynamic MBean exposing the <br>
perf counters. <br>
<br>
with webrev at, <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ehb/8007141/webrev.00/">http://cr.openjdk.java.net/~hb/8007141/webrev.00/</a>
<br>
<br>
Appreciate if I can get inputs for below. <br>
<br>
1. Location of HotSpotPerfCounterMBean. It is located at <br>
src/jdk.management/share/classes/com/sun/management/internal. It
can be <br>
moved to src/jdk.management/share/classes/com/sun/management if
required. <br>
<br>
2. Javadoc comments for HotSpotPerfCounterMBean. Not sure if it
is adequate. <br>
<br>
3. Description for each attribute of MBean. I am using
getUnits(), <br>
getVariability(), and getFlags() for description. I am not sure
if that <br>
is the right description. Appreciate inputs from someone who
knows perf <br>
counters well. <br>
<br>
Thanks <br>
<br>
Harsha <br>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic