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
