RFR : JDK-8024352 - MBeanOperationInfo accepts any int value as "impact"

[Ujwal Vangapally]

Thanks for the suggestions Roger, Mandy.

below is webrev incorporating review comments.

http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.02/

CSR: https://bugs.openjdk.java.net/browse/JDK-8190197

Thanks,

Ujwal

On 11/7/2017 10:48 PM, Ujwal Vangapally wrote:
> Hi Roger,
>
> kindly see my understanding below.
>
>
> On 11/7/2017 9:10 PM, Roger Riggs wrote:
>> Hi Ujwal,
>>
>> MBeanOperationInfo:163:
>> Since the values are fixed, you could more concisely just compare 
>> impact >=0 and impact <= UNKNOWN.
> As there are only 4 constants, I thought it would be better to include 
> them directly instead of depending on there values.
>
> If it is a good practice to do it by comparing there values I will do 
> it that way.
>>
>> 257/263:  I don't see a reason to change the toString in the default 
>> case for getImpact().
>>
> As impact can't have any other value than 4 constants, we don't need 
> the default case any more.
>>
>> A suggestion would be to introduce an Enum for the action values and 
>> the corresponding new
>> method; perhaps deprecating the current method (or not).
>> The enum would use the same values as currently and so internally the 
>> implementation does not
>> change significantly.
> Kindly clarify.
> As it changes the constructor signature if we introduce a Enum,
> but as we can solve the issue by throwing IAE, do we still need to 
> introduce Enum and change method signature.
>
> Thanks,
> Ujwal.
>>
>> $.02, Roger
>>
>>
>> On 11/7/2017 6:05 AM, Ujwal Vangapally wrote:
>>> Kindly review the fix for bug below.
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8024352
>>>
>>> webrev : 
>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.00/
>>>
>>> Thanks,
>>>
>>> Ujwal.
>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20171109/8ffa7654/attachment.html>