jmx-dev Codereview: JDK-8065764 javax/management/monitor/CounterMonitorTest.java hangs

[Jaroslav Bachorik]

On 12/02/2014 06:56 PM, shanliang wrote:
> Jaroslav Bachorik wrote:
>> On 12/02/2014 05:22 PM, shanliang wrote:
>>> Jaroslav Bachorik wrote:
>>>> On 12/02/2014 04:22 PM, shanliang wrote:
>>>>> Jaroslav Bachorik wrote:
>>>>>> On 12/02/2014 02:40 PM, shanliang wrote:
>>>>>>> Jaroslav Bachorik wrote:
>>>>>>>> On 12/01/2014 02:50 PM, shanliang wrote:
>>>>>>>>> Hi,
>>>>>>>>> please review this test bug fix:
>>>>>>>>>
>>>>>>>>> webrev:
>>>>>>>>> http://cr.openjdk.java.net/~sjiang/JDK-8065764/00/
>>>>>>>>>
>>>>>>>>> bug:
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8065764
>>>>>>>>>
>>>>>>>>
>>>>>>>> test/javax/management/monitor/CounterMonitorTest.java
>>>>>>>> L61 - observedValue could be Integer
>>>>>>> Could be, but not make difference, observedValue is only used
>>>>>>> with the
>>>>>>> operation "==", like
>>>>>>>     while (value != observedValue)
>>>>>>
>>>>>> In that case having a correct type would make even more sense for the
>>>>>> readability.
>>>>> Agree in general:) but the variable "count" is declared as "Object" in
>>>>> StdObservedObject, and "observedValue" is used to save observed
>>>>> "count"
>>>>> value, so better to keep the same type for "more readability"? and
>>>>> avoid
>>>>> a casting when setting "observedValue"
>>>>
>>>> Any reason 'count' is declared as Object? The value is effectively an
>>>> integer value - only integer values are ever assigned to it. All the
>>>> operations silently suppose this would be an integer and yet the type
>>>> information is thrown away and the data is stored as Object. Feels
>>>> very strange ...
>>> I think a possible reason is that the CounterMonitor supports only
>>> Number type for comparing, and it accepts any type but detects whether
>>> the observed variable could be "matched" to Number, pass an Object type
>>> verifies this capability.
>>
>> Hm, could you direct me to the code doing this? 'setAttribute' method
>> seems to be inherited from 'Monitor' class and does not do any
>> additional checks. Then 'Monitor.monitor()' uses 'getAttribute'
>> (again, not overridden) to get back to the attribute :(
> ConterMonitor overrides Monitor.isComparableTypeValid(...)
>
> This method will be called by Monitor at the monitoring time and a
> Notification with the type "jmx.monitor.error.type" will be sent out if
> the observed attribute is not of type "Integer".
>
> Yes the monitor might have to do type check with the method:
>     Monitor.setObservedAttribute("NbObjects");
>
> but the Javadoc declares only:
>     Throws: IllegalArgumentException - The specified attribute is null.
>
> maybe it is not sure that a monitor could do type check at setting time,
> for example if the MBeanInfo is null and getAttributeValue() returns null.

I still don't see how using Object instead of Integer type could check 
this functionality. But if you insist on keeping it this way I won't 
block fixing this failing test any further. Reviewed.

-JB-

>
> Shanliang
>>
>> Thanks,
>>
>> -JB-
>>
>>>>
>>>>>>
>>>>>>>> L225-238 - you could replace this block with the usage of Phaser
>>>>>>>> (if
>>>>>>>> you'd do that you could completely remove 'observedValue')
>>>>>>> Not sure that it is a good idea to remove "observedValue" by using
>>>>>>> Phaser, yes using Phaser can tell when the monitor does an
>>>>>>> observation,
>>>>>>> but it is better to know which count value the monitor observed, in
>>>>>>> case
>>>>>>> the thread was waked up accidentally.
>>>>>>
>>>>>> I don't think phaser can be confused by spurious wake-ups.
>>>>>>
>>>>>> If you want to keep the current way of synchronizing then you should
>>>>>> make the 'count' variable volatile (L77) - it might be read and
>>>>>> written to from different threads.
>>>>> Phaser should not make a false wakeup, but use "observedValue" can
>>>>> make
>>>>> sure that the observation happens on the right "count" value.
>>>>> Yes "count" should be declared as "volatile".
>>>>>
>>>>> Here is the new version:
>>>>>     http://cr.openjdk.java.net/~sjiang/JDK-8065764/01/
>>>>
>>>> Still using 'derivedGange' instead of 'derivedGauge' :(
>>> Oh I will change it before pushing, if no other modification.
>>>
>>> Thanks,
>>> Shanliang
>>>>
>>>> -JB-
>>>>
>>>>>
>>>>> Thanks,
>>>>> Shanliang
>>>>>
>>>>>>
>>>>>> -JB-
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Shanliang
>>>>>>>>
>>>>>>>> -JB-
>>>>>>>>
>>>>>>>>
>>>>>>>>> The test tested the mode "difference", according to the Javadoc:
>>>>>>>>>       If the counter difference mode is used, the value of the
>>>>>>>>> derived
>>>>>>>>> gauge is calculated as the difference between the observed counter
>>>>>>>>> values for two successive observations.
>>>>>>>>>
>>>>>>>>> The test set the first value and then waited 2 times of
>>>>>>>>> granularityperiod at line 171, hoped that the monitor would get
>>>>>>>>> the
>>>>>>>>> first observation during this waiting time, but the test could
>>>>>>>>> fail
>>>>>>>>> because granularityperiod * 2 was not enough and the test did the
>>>>>>>>> second
>>>>>>>>> set before the monitor did the first observation.
>>>>>>>>>
>>>>>>>>> It is easy to make the test timeout by commenting out the line
>>>>>>>>> 171.
>>>>>>>>>
>>>>>>>>> The proposed solution is to get informed when the monitor did
>>>>>>>>> observation on calling:
>>>>>>>>>     StdObservedObject.getNbObjects();
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Shanliang
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>