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

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Ujwal Vangapally
Reply | Threaded
Open this post in threaded view
|

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

roger riggs
Hi Ujwal,

MBeanOperationInfo:163:
Since the values are fixed, you could more concisely just compare impact
 >=0 and impact <= UNKNOWN.

257/263:  I don't see a reason to change the toString in the default
case for getImpact().


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.

$.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.
>
>

Reply | Threaded
Open this post in threaded view
|

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

Ujwal Vangapally
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.
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: 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.





Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs
Hi Ujwal,

Give that there are only 4 legal values to check then maybe
you could check all of them in the test (and not simply 2).

best regards,

-- daniel


On 08/11/2017 18:34, Ujwal Vangapally wrote:

> 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.
>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Ujwal Vangapally
Thanks for the Review Daniel, made changes as suggested.

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

Ujwal.


On 11/9/2017 3:37 PM, Daniel Fuchs wrote:

> Hi Ujwal,
>
> Give that there are only 4 legal values to check then maybe
> you could check all of them in the test (and not simply 2).
>
> best regards,
>
> -- daniel
>
>
> On 08/11/2017 18:34, Ujwal Vangapally wrote:
>> 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.
>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs
On 09/11/2017 10:40, Ujwal Vangapally wrote:
> Thanks for the Review Daniel, made changes as suggested.
>
> webrev :
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.03/

Looks good to me.

-- daniel

>
> Ujwal.
>
Reply | Threaded
Open this post in threaded view
|

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

roger riggs
+1

Looks good,

Thanks, Roger


On 11/9/17 6:34 AM, Daniel Fuchs wrote:

> On 09/11/2017 10:40, Ujwal Vangapally wrote:
>> Thanks for the Review Daniel, made changes as suggested.
>>
>> webrev :
>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.03/
>
> Looks good to me.
>
> -- daniel
>
>>
>> Ujwal.
>>

Reply | Threaded
Open this post in threaded view
|

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

Ujwal Vangapally
Thanks for the Review Daniel, Roger.

Ujwal


On 11/9/2017 7:52 PM, Roger Riggs wrote:

> +1
>
> Looks good,
>
> Thanks, Roger
>
>
> On 11/9/17 6:34 AM, Daniel Fuchs wrote:
>> On 09/11/2017 10:40, Ujwal Vangapally wrote:
>>> Thanks for the Review Daniel, made changes as suggested.
>>>
>>> webrev :
>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.03/
>>
>> Looks good to me.
>>
>> -- daniel
>>
>>>
>>> Ujwal.
>>>
>

Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung
In reply to this post by Ujwal Vangapally


On 11/9/17 2:40 AM, Ujwal Vangapally wrote:
Thanks for the Review Daniel, made changes as suggested.

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


Looks good.

Minor comment: in the new test, it can fold some of the println together e.g. line 81 can be merged with line 39 to include the value being passed.  Similarly for the println in the main method.

Mandy

Reply | Threaded
Open this post in threaded view
|

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

Ujwal Vangapally
Thanks for the review Mandy,

kindly check if this version is better.

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

Ujwal


On 11/9/2017 9:10 PM, mandy chung wrote:

>
>
> On 11/9/17 2:40 AM, Ujwal Vangapally wrote:
>> Thanks for the Review Daniel, made changes as suggested.
>>
>> webrev :
>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.03/
>>
>
> Looks good.
>
> Minor comment: in the new test, it can fold some of the println
> together e.g. line 81 can be merged with line 39 to include the value
> being passed.  Similarly for the println in the main method.
>
> Mandy
>

Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung


On 11/9/17 9:03 AM, Ujwal Vangapally wrote:
Thanks for the review Mandy,

kindly check if this version is better.

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

Looks okay.

Mandy
Reply | Threaded
Open this post in threaded view
|

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

Ujwal Vangapally
In reply to this post by Ujwal Vangapally
kindly review the updated webrev including changes to
MBeanInfoHashCodeNPETest.java

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

Thanks,

Ujwal.


On 11/9/2017 10:33 PM, Ujwal Vangapally wrote:

> Thanks for the review Mandy,
>
> kindly check if this version is better.
>
> webrev :
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.04/
>
> Ujwal
>
>
> On 11/9/2017 9:10 PM, mandy chung wrote:
>>
>>
>> On 11/9/17 2:40 AM, Ujwal Vangapally wrote:
>>> Thanks for the Review Daniel, made changes as suggested.
>>>
>>> webrev :
>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.03/
>>>
>>
>> Looks good.
>>
>> Minor comment: in the new test, it can fold some of the println
>> together e.g. line 81 can be merged with line 39 to include the value
>> being passed.  Similarly for the println in the main method.
>>
>> Mandy
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs
Hi Ujwal,

Still looks good to me.

best regards,

-- daniel

On 15/11/2017 13:18, Ujwal Vangapally wrote:

> kindly review the updated webrev including changes to
> MBeanInfoHashCodeNPETest.java
>
> webrev :
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.05/
>
> Thanks,
>
> Ujwal.
>
>
> On 11/9/2017 10:33 PM, Ujwal Vangapally wrote:
>> Thanks for the review Mandy,
>>
>> kindly check if this version is better.
>>
>> webrev :
>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.04/
>>
>> Ujwal
>>
>>
>> On 11/9/2017 9:10 PM, mandy chung wrote:
>>>
>>>
>>> On 11/9/17 2:40 AM, Ujwal Vangapally wrote:
>>>> Thanks for the Review Daniel, made changes as suggested.
>>>>
>>>> webrev :
>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8024352/webrev.03/
>>>>
>>>
>>> Looks good.
>>>
>>> Minor comment: in the new test, it can fold some of the println
>>> together e.g. line 81 can be merged with line 39 to include the value
>>> being passed.  Similarly for the println in the main method.
>>>
>>> Mandy
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

roger riggs
+1

On 11/15/2017 10:09 AM, Daniel Fuchs wrote:
Hi Ujwal,

Still looks good to me.

best regards,

-- daniel

On 15/11/2017 13:18, Ujwal Vangapally wrote:
kindly review the updated webrev including changes to MBeanInfoHashCodeNPETest.java

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

Thanks,

Ujwal.


On 11/9/2017 10:33 PM, Ujwal Vangapally wrote:
Thanks for the review Mandy,

kindly check if this version is better.

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

Ujwal


On 11/9/2017 9:10 PM, mandy chung wrote:


On 11/9/17 2:40 AM, Ujwal Vangapally wrote:
Thanks for the Review Daniel, made changes as suggested.

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


Looks good.

Minor comment: in the new test, it can fold some of the println together e.g. line 81 can be merged with line 39 to include the value being passed.  Similarly for the println in the main method.

Mandy