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)" <[email protected]> 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 > >
