geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Jencks <david_jen...@yahoo.com>
Subject Re: [jira] Commented: (GERONIMO-2135) Improve the ActiveMQ GBeans
Date Thu, 22 Jun 2006 09:08:16 GMT
This patch addresses my major concerns. I consider it
a bug fix and thus not requiring further voting beyond
my previous +1, but in case anyone disagrees, I've
applied it and tested it to the best of my abilities
and vote +1.

thanks
david jencks


--- "Gianny Damour (JIRA)" <dev@geronimo.apache.org>
wrote:

>     [
>
http://issues.apache.org/jira/browse/GERONIMO-2135?page=comments#action_12417237
> ] 
> 
> Gianny Damour commented on GERONIMO-2135:
> -----------------------------------------
> 
> I had a look to the patch and vote +1 for it. Some
> more details:
> 1. is fixed.
> 2. is not fixed.
> 3. is partially fixed
> 4. is not fixed
> 5., 6. and 7. do not know
> 8. is fixed. Also, it seems that we also avoid
> "abstract" from methods in interfaces (one occurence
> in BrokerServiceGBean).
> 
> 
> > Improve the ActiveMQ GBeans
> > ---------------------------
> >
> >          Key: GERONIMO-2135
> >          URL:
> http://issues.apache.org/jira/browse/GERONIMO-2135
> >      Project: Geronimo
> >         Type: Improvement
> >     Security: public(Regular issues) 
> >   Components: ActiveMQ
> >     Reporter: Hiram Chirino
> >     Assignee: Hiram Chirino
> >      Fix For: 1.2
> >  Attachments: GERONIMO-2135.patch
> >
> > Suggestions by David Jencks:
> > I think that this gbean adaptation code should be
> in geronimo rather
> > than amq.  I'm OK with applying it as is but would
> prefer some issues
> > to be addressed first or, even better, 
> immediately after the
> > transfer (assuming it is done with svn mv).
> > 1. DataSourceReference should be replaced by the
> geronimo class that
> > does the same thing, ConnectionFactorySource.
> > 2. I think it would be preferable to get the
> module/configuration
> > classloader in the constructor as a magic
> attribute and use it in
> > BrokerServiceGBeanImpl.doStart rather than the
> classloader of
> > BrokerServiceGBeanImpl.
> > 3. Same for TransportConnectorGBeanImpl.
> > 4. This is a question, not really an issue, about
> this code:
> > +    protected TransportConnector
> createBrokerConnector(String url)
> > throws Exception {
> > +        return
>
brokerService.getBrokerContainer().addConnector(url);
> > +    }
> > To me it seems like this code is combining the
> functions of factory
> > object and container.  Is this necessary and
> appropriate?  I'd be
> > more comfortable with
> > Connector connector =
> ConnectorFactory.createConnector(url);
> >
>
brokerService.getBrokerContainer().addConnector(connector);
> > I find that the combination style typically
> creates problems whenever
> > trying to extend stuff, say by wrapping the
> connector.  What do you
> > think?
> > 5. hardcoding the protocols in
> ActiveMQManagerGBean seems like a
> > temporary expedient at best.
> > 6. javadoc on public JMSConnector addConnector(
> ... in the manager
> > gbean seems wrong... does not appear to return an
> object name.
> > 7. Typo and innaccuracies in the first
> package.html... this stuff is
> > only going to work in geronimo, jsr77/88 is not
> enough.
> > 8. I'm not sure exactly what our official policy
> is but I prefer to
> > remove "public" from methods in interfaces since
> it is the only
> > choice and implied.
> 
> -- 
> This message is automatically generated by JIRA.
> -
> If you think it was sent incorrectly contact one of
> the administrators:
>   
>
http://issues.apache.org/jira/secure/Administrators.jspa
> -
> For more information on JIRA, see:
>    http://www.atlassian.com/software/jira
> 
> 


Mime
View raw message