admin@glassfish.java.net

Re: Request for review

From: Lloyd L Chambers <Lloyd.Chambers_at_Sun.COM>
Date: Thu, 08 Nov 2007 11:51:14 -0800

Feedback--

This implementation is unacceptable. Its use of static (global)
variables is inherently problematic in any environment with more than
one thread (all environments!). Who's to say which value of
'descriptionClass' takes precedence? Apparently it's "last thread
wins", something which is useless and inappropriate in a threaded
environment, a broken idiom which is never appropriate.

That major issue aside, as-coded it is not thread safe. While I
strongly recommend fixing the inherently broken design, at the very
least the static variable should be 'volatile'.

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