admin@glassfish.java.net

Re: CODE REVIEW: minor bug fixes for 9.1.1 AMX unit test failures

From: Lloyd L Chambers <Lloyd.Chambers_at_Sun.COM>
Date: Mon, 03 Dec 2007 10:58:16 -0800

Nandini,

Nothing is ever sure. But there is a clear implementation mistake,
and I judge it reasonable and safe to remove it.

In fact, though there is extra MBeanInfo, the method implementations
do not change in any way. And it's not even clear that a client can
deterministically "see" the latter info, since it's merged after the
originals (say if a search by Attribute name is made linearly through
the MBeanAttributeInfo, as it must be for a List.

Lloyd

On Dec 3, 2007, at 10:51 AM, Nandini Ektare wrote:

> Lloyd,
>
> Lloyd L Chambers wrote:
>> Nandini,
>>
>> All MBeanInfo is by default implemented by AMXImplBase; the
>> MBeanInfo is generated dynamically using the interface for the
>> particular MBean, done generically for all AMX MBeans. A few rare
>> subclasses override it and generate their own MBeanInfo. This was
>> one of them, but it was done wrong, creating duplicate entries.
>> And there really is no need to do so; duplicating "state" for
>> example as "Integer" when the spec calls for "int" seems quite
>> inappropriate.
> My point was that, if you are not sure of where the extra
> information is being used, it would be risky to simply remove it
> (agreed that duplicates need to be removed regardless).
>
> Looks like you are sure, it is not required. Please go ahead.
>
> Thanks
> Nandini
>>
>> Lloyd
>>
>> On Nov 30, 2007, at 5:28 PM, Nandini Ektare wrote:
>>
>>> Lloyd L Chambers wrote:
>>>> Thanks Nandini.
>>>>
>>>> For #2 ("bug in which the J2EEServer MBeanInfo has duplicate
>>>> items in it"), I've commented out the explicit code that
>>>> modifies the default MBeanInfo; it was merging additional items
>>>> over the ones already in the MBeanInfo. So I'm just removing
>>>> code for that one.
>>>>
>>>> See code below; it's commented out now. The problem call is
>>>> getMBeanInfo() which is merging additional infos into already
>>>> existing ones, creating duplicates. There is a slight
>>>> difference (Integer vs int, wrong per spec) and
>>>> MBeanOperationInfo.ACTION, which no one cares about. So it's
>>>> safe to get rid of it.
>>> So you mean the attribute "server state" and operations
>>> "start"/"stop"/"startRecursive" will be fetched in the base
>>> class's getMBeanInfo() call?
>>>
>>> Nandini
>>>>
>>>> Lloyd
>>>>
>>>>
>>>> /*
>>>> private MBeanInfo mMBeanInfo = null;
>>>>
>>>> public synchronized MBeanInfo
>>>> getMBeanInfo()
>>>> {
>>>> // compute the MBeanInfo only once!
>>>> if ( mMBeanInfo == null )
>>>> {
>>>> final MBeanInfo superMBeanInfo =
>>>> super.getMBeanInfo();
>>>> mMBeanInfo = new MBeanInfo(
>>>> superMBeanInfo.getClassName(),
>>>> superMBeanInfo.getDescription(),
>>>> mergeAttributeInfos(superMBeanInfo.getAttributes
>>>> (), getMBeanAttributeInfo()),
>>>> superMBeanInfo.getConstructors(),
>>>> mergeOperationInfos(superMBeanInfo.getOperations
>>>> (), getMBeanOperationInfo()),
>>>> superMBeanInfo.getNotifications() );
>>>> }
>>>> return mMBeanInfo;
>>>> }
>>>>
>>>> private MBeanAttributeInfo[]
>>>> mergeAttributeInfos(
>>>> MBeanAttributeInfo[] infos1,
>>>> MBeanAttributeInfo[] infos2 )
>>>> {
>>>> final MBeanAttributeInfo[] infos =
>>>> new MBeanAttributeInfo[ infos1.length +
>>>> infos2.length ];
>>>> System.arraycopy( infos1, 0, infos, 0,
>>>> infos1.length );
>>>> System.arraycopy( infos2, 0, infos, infos1.length,
>>>> infos2.length );
>>>> return( infos );
>>>> }
>>>>
>>>>
>>>> private MBeanAttributeInfo[]
>>>> getMBeanAttributeInfo()
>>>> {
>>>> MBeanAttributeInfo[] dAttributes = new MBeanAttributeInfo
>>>> [1];
>>>> dAttributes[0] = new MBeanAttributeInfo("state",
>>>>
>>>> "java.lang.Integer",
>>>> "server state",
>>>> true,
>>>> false,
>>>> false);
>>>> return dAttributes;
>>>> }
>>>> */
>>>>
>>>>
>>>> /* private MBeanOperationInfo[]
>>>> mergeOperationInfos(
>>>> MBeanOperationInfo[] infos1,
>>>> MBeanOperationInfo[] infos2 )
>>>> {
>>>> final MBeanOperationInfo[] infos =
>>>> new MBeanOperationInfo[ infos1.length +
>>>> infos2.length ];
>>>> System.arraycopy( infos1, 0, infos, 0,
>>>> infos1.length );
>>>> System.arraycopy( infos2, 0, infos, infos1.length,
>>>> infos2.length );
>>>> return( infos );
>>>> }
>>>> */
>>>>
>>>> /*
>>>> private MBeanOperationInfo[]
>>>> getMBeanOperationInfo()
>>>> {
>>>> MBeanOperationInfo[] dOperations = new MBeanOperationInfo
>>>> [3];
>>>> dOperations[0] = new MBeanOperationInfo("start",
>>>> "start server
>>>> instance",
>>>> null,
>>>> "void",
>>>>
>>>> MBeanOperationInfo.ACTION);
>>>> dOperations[1] = new MBeanOperationInfo("stop",
>>>> "stop server
>>>> instance",
>>>> null,
>>>> "void",
>>>>
>>>> MBeanOperationInfo.ACTION);
>>>> dOperations[2] = new MBeanOperationInfo("startRecursive",
>>>> "start server
>>>> instance",
>>>> null,
>>>> "void",
>>>>
>>>> MBeanOperationInfo.ACTION);
>>>> return dOperations;
>>>> }
>>>> */
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Nov 30, 2007, at 3:39 PM, Nandini Ektare wrote:
>>>>
>>>>> Lloyd L Chambers wrote:
>>>>> This is for 9.1.1.
>>>>>
>>>>> These two fixes address AMX unit test failures:
>>>>>
>>>>> 1. A bug in which "jvm.log" is returned with the list of
>>>>> server log files.
>>>>> Looks fine.
>>>>> 2. A bug in which the J2EEServer MBeanInfo has duplicate items
>>>>> in it.
>>>>> Is that the output of cvs diff -u ? It's not easy to read those
>>>>> diffs. Seems like you have done fixes to mergeOperationInfos
>>>>> and mergeAttributeInfos.
>>>>
>>>> ---
>>>> Lloyd L Chambers
>>>> lloyd.chambers_at_sun.com
>>>> Sun Microsystems, Inc
>>>>
>>>>
>>>>
>>>> -------------------------------------------------------------------
>>>> --
>>>> To unsubscribe, e-mail: admin-unsubscribe_at_glassfish.dev.java.net
>>>> For additional commands, e-mail: admin-help_at_glassfish.dev.java.net
>>>>
>>>
>>> --------------------------------------------------------------------
>>> -
>>> To unsubscribe, e-mail: admin-unsubscribe_at_glassfish.dev.java.net
>>> For additional commands, e-mail: admin-help_at_glassfish.dev.java.net
>>>
>>
>> ---
>> Lloyd L Chambers
>> lloyd.chambers_at_sun.com
>> Sun Microsystems, Inc
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: admin-unsubscribe_at_glassfish.dev.java.net
>> For additional commands, e-mail: admin-help_at_glassfish.dev.java.net
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: admin-unsubscribe_at_glassfish.dev.java.net
> For additional commands, e-mail: admin-help_at_glassfish.dev.java.net
>

---
Lloyd L Chambers
lloyd.chambers_at_sun.com
Sun Microsystems, Inc