Hi Lloyd,
Thanks for the quick feedback, will incorporate those and checkin..
-Yamini
Lloyd L Chambers wrote:
> Yamini,
>
> Looks good. Some minor things:
>
> 1) Generic types make casts unecessary and make for cleaner code:
> for( final ResourceBundled resourceBundle : resourceBundles ) { ... }
>
> Then you won't need a cast, or Iterator either.
>
> 2) A comment is worthwhile, indicating that 'Vector' is used for its
> thread-safe properties.
>
> 3) While iterating/looping, addition of a resource bundle might do
> what? (cause an exception, work fine?). A comment is in order:
> "If {_at_link #addResourceBundle} is called while getting a description,
> ...".
>
> You could call toArray() I suppose, that would preclude an issue (I
> think--check what Vector javadoc says).
>
> Thanks,
> Lloyd
>
> On Nov 14, 2007, at 9:02 PM, Yamini K B wrote:
>
>> Hi Lloyd,
>>
>> Implemented your suggestion as follows, please review the same:
>>
>> 1. StatsDescription will load a default resource bundle.
>> 2. Exposed a method to add resource bundles externally.
>> 3. Exposed getInstance() to allow for only one instance of this class.
>>
>> Thanks,
>> -Yamini
>>
>> public class StatsDescriptionHelper {
>> private final Vector<ResourceBundle> resourceBundles = new
>> Vector<ResourceBundle>();
>> public static final String NO_DESCRIPTION_AVAILABLE = "No Description
>> was available";
>> private static final String DEFAULT_PACKAGE =
>> StatsDescriptionHelper.class.getPackage().getName();
>> private static final String DEFAULT_FILE = "LocalStrings";
>> private static final StatsDescriptionHelper sdh = new
>> StatsDescriptionHelper();
>>
>> public StatsDescriptionHelper() {
>> ResourceBundle resourceBundle =
>> ResourceBundle.getBundle(DEFAULT_PACKAGE + "." + DEFAULT_FILE);
>> resourceBundles.add(resourceBundle);
>> }
>>
>> public static StatsDescriptionHelper getInstance() {
>> return sdh;
>> }
>>
>> /*
>> * Add a resource bundle
>> */
>> public void addResourceBundle(ResourceBundle rb)
>> {
>> resourceBundles.add(rb);
>> }
>> /*
>> * Returns the localized description
>> */
>> public String getDescription(String key)
>> {
>> Iterator it = resourceBundles.iterator();
>> String value = NO_DESCRIPTION_AVAILABLE;
>> while (it.hasNext()) {
>> ResourceBundle resourceBundle = (ResourceBundle)it.next();
>> try {
>> value = resourceBundle.getString(key);
>> break;
>> }
>> catch (MissingResourceException mre) {
>> // continue search in next resource bundle
>> }
>> }
>> return value;
>> }
>> }
>>
>>
>>
>>
>> Lloyd L Chambers wrote On 11/09/07 22:47,:
>>
>>> Yamini,
>>>
>>> This is the unmodified class I see:
>>>
>>> class StatsDescriptionHelper {
>>> private LocalStringManager mgr;
>>> static final String NO_DESCRIPTION_AVAILABLE="No Description was
>>> available";
>>> StatsDescriptionHelper(){
>>> mgr = new LocalStringManagerImpl(this.getClass());
>>> }
>>> String getDescription(java.lang.String name) {
>>> return mgr.getLocalString(this.getClass(), name,
>>> NO_DESCRIPTION_AVAILABLE);
>>> }
>>> }
>>>
>>> Comments:
>>> - 'mgr' should be 'final'.
>>> - StatsDescriptionHelper is instantiated multiple times, but actual
>>> use appears to be as a singleton; all clients apparently should be
>>> accessing exactly the same strings. So make it a singleton with a
>>> private constructor, a 'private static final' instance, and a public
>>> getInstance() method.
>>> - It's unclear (in your modified code) whether setDescriptionClass()
>>> will be called before StatsDescriptionHelper is instantiated. I see
>>> no way of guaranteeing that, so that's a bug.
>>>
>>> The apparent intent is to set the 'descriptionClass' static variable
>>> externally and therefore cause the create() method to use some
>>> subclass of StatsDescriptionHelper, obtained by Class.forName().
>>>
>>> Thinking about this problem in general, such a "helper" class might
>>> in general need to find strings from more than one place eg search
>>> locations A, B, C, etc in order until a String is found. That would
>>> be a general solution, and a dynamic one too, better for a dynamic
>>> environment in which modules might load and unload.
>>>
>>> My suggestion is this: create a general purpose base class (of which
>>> StatsDescriptionHelper is a subclass) that allows the addition of
>>> more than one resource bundle (removal is probably unnecessary). Any
>>> subclass can insert its default resource bundle via super(), and
>>> external code can insert (in a thread-safe way) additional resource
>>> bundle(s) dynamically. With this capability, the SIP code can do:
>>>
>>> StatsDescriptionHelper.getInstance().addResourceBundle( myBundle1 );
>>> StatsDescriptionHelper.getInstance().addResourceBundle( myBundle2 );
>>> etc
>>>
>>> Of course, this has global and dynamic effect for all clients of
>>> StatsDescriptionHelper, but this is apparently the desired semantic.
>>>
>>> The internal list of resource bundles will need to be dealt with in
>>> a thread-safe manner of course.
>>>
>>> Lloyd
>>>
>>> On Nov 6, 2007, at 9:30 AM, Yamini K B wrote:
>>>
>>>>
>>>> Description: GlassFish monitoring module stores the description
>>>> texts for all the statistics in LocalStrings.properties packaged
>>>> under com.sun.enterprise.admin.monitor.registry.spi. There is a
>>>> StatsDescriptionHelper class within the same package that looks up
>>>> for the description text during the registration of the monitoring
>>>> objects. Recently, we extended GlassFish monitoring to load
>>>> external monitoring modules (to be able to support SailFin
>>>> monitoring) The problem is that description texts fail to show up
>>>> for the SIP monitoring statistics, the reason being that
>>>> StatsDescriptionHelper tries to load the resource bundle from the
>>>> same package as itself. So the requirement is that
>>>> StatsDescriptionHelper should be able to pick up description texts
>>>> for modules loaded externally.
>>>>
>>>> Solution: Provided a hook in StatsDescriptionHelper to load the
>>>> resource bundle from the caller class. Attached diffs contain the
>>>> following changes:
>>>>
>>>> 1. Exposed a method setDescriptionClass() that will be called from
>>>> some external module.
>>>> 2. Create the StatsDescriptionHelper based on the the description
>>>> class.
>>>>
>>>> To make this work end-to-end the following changes was done on
>>>> SailFin side:
>>>>
>>>> 1. Created an empty class SipStatsDescriptionHelper which extends
>>>> StatsDescriptionHelper.
>>>> 2. Modified SipMonitoringManagerImpl (which is the initialization
>>>> class for SIP monitoring) to set the description class just before
>>>> registering the SIP stats. The stats description class needs to be
>>>> unset as soon as the job is done since we wouldn't want this to be
>>>> used when some GlassFish monitoring registration/unregistration
>>>> happens.
>>>> 3. Created LocalStrings.properties within the same package as
>>>> SipStatsDescriptionHelper.
>>>>
>>>> Would appreciate your comments on the suggested changes.
>>>>
>>>> Thanks,
>>>> -Yamini
>>>>
>>>>
>>>>
>>>> Index: StatisticWorkaround.java
>>>> ===================================================================
>>>> RCS file: /cvs/glassfish/admin/monitor/src/java/com/sun/enterprise/
>>>> admin/monitor/registry/spi/StatisticWorkaround.java,v
>>>> retrieving revision 1.5
>>>> diff -u -r1.5 StatisticWorkaround.java
>>>> --- StatisticWorkaround.java 5 May 2007 05:24:19 -0000 1.5
>>>> +++ StatisticWorkaround.java 5 Nov 2007 15:40:20 -0000
>>>> @@ -64,7 +64,7 @@
>>>> */
>>>>
>>>> final class StatisticWorkaround {
>>>> - private static final StatsDescriptionHelper helper = new
>>>> StatsDescriptionHelper();
>>>> + private static final StatsDescriptionHelper helper =
>>>> StatsDescriptionHelper.create();
>>>> private static Logger logger = Logger.getLogger
>>>> (AdminConstants.kLoggerName);
>>>> static final Statistic[] populateDescriptions(final Statistic[]
>>>> from) {
>>>> if (from == null) {
>>>> Index: StatsMediatorImpl.java
>>>> ===================================================================
>>>> RCS file: /cvs/glassfish/admin/monitor/src/java/com/sun/enterprise/
>>>> admin/monitor/registry/spi/StatsMediatorImpl.java,v
>>>> retrieving revision 1.7
>>>> diff -u -r1.7 StatsMediatorImpl.java
>>>> --- StatsMediatorImpl.java 5 May 2007 05:24:19 -0000 1.7
>>>> +++ StatsMediatorImpl.java 5 Nov 2007 15:40:21 -0000
>>>> @@ -88,7 +88,7 @@
>>>> private final String OLD_DELIMITER = "_";
>>>> // some JTA specific constants
>>>> private Map opsMap;
>>>> - private final StatsDescriptionHelper helper = new
>>>> StatsDescriptionHelper();
>>>> + private final StatsDescriptionHelper helper =
>>>> StatsDescriptionHelper.create();
>>>> private final String DESCRIPTION_GETTER = "getDescription";
>>>> private static final String DOTTED_NAME = "dotted-name";
>>>>
>>>> Index: StatsDescriptionHelper.java
>>>> ===================================================================
>>>> RCS file: /cvs/glassfish/admin/monitor/src/java/com/sun/enterprise/
>>>> admin/monitor/registry/spi/StatsDescriptionHelper.java,v
>>>> retrieving revision 1.4
>>>> diff -u -r1.4 StatsDescriptionHelper.java
>>>> --- StatsDescriptionHelper.java 5 May 2007 05:24:19 -0000 1.4
>>>> +++ StatsDescriptionHelper.java 6 Nov 2007 16:56:59 -0000
>>>> @@ -41,6 +41,7 @@
>>>> */
>>>>
>>>> package com.sun.enterprise.admin.monitor.registry.spi;
>>>> +
>>>> import com.sun.enterprise.util.LocalStringManager;
>>>> import com.sun.enterprise.util.LocalStringManagerImpl;
>>>> /**
>>>> @@ -48,14 +49,37 @@
>>>> * given statistic.
>>>> * @author Shreedhar Ganapathy<mailto:shreedhar.ganapathy_at_sun.com>
>>>> */
>>>> -class StatsDescriptionHelper {
>>>> - private LocalStringManager mgr;
>>>> +public class StatsDescriptionHelper {
>>>> + protected LocalStringManager mgr;
>>>> static final String NO_DESCRIPTION_AVAILABLE="No Description was
>>>> available";
>>>> + static String descriptionClass = null;
>>>> - StatsDescriptionHelper(){
>>>> + public StatsDescriptionHelper(){
>>>> mgr = new LocalStringManagerImpl(this.getClass());
>>>> }
>>>> + public static StatsDescriptionHelper create() {
>>>> + if (descriptionClass == null) {
>>>> + return new StatsDescriptionHelper();
>>>> + } else {
>>>> + StatsDescriptionHelper sdh = null;
>>>> + try {
>>>> + sdh = (StatsDescriptionHelper) Class.forName
>>>> (descriptionClass).newInstance();
>>>> + } catch (Exception e) {
>>>> + // Fall back on default in case of error
>>>> + sdh = new StatsDescriptionHelper();
>>>> + }
>>>> + return sdh;
>>>> + }
>>>> + }
>>>> +
>>>> + /*
>>>> + * Set the description class
>>>> + */
>>>> + public static void setDescriptionClass(String c) {
>>>> + descriptionClass = c;
>>>> + }
>>>> +
>>>> String getDescription(java.lang.String name) {
>>>> return mgr.getLocalString(this.getClass(), name,
>>>> NO_DESCRIPTION_AVAILABLE);
>>>> }
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> 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
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: admin-unsubscribe_at_glassfish.dev.java.net
> For additional commands, e-mail: admin-help_at_glassfish.dev.java.net
>