qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Stitcher" <astitc...@apache.org>
Subject Re: Review Request: Recreation of JCA Resource Adapter review
Date Fri, 15 Jul 2011 16:05:24 GMT


> On 2011-07-15 15:40:16, Robbie Gemmell wrote:
> > The apparent JBoss 'dependency' for this seems less than ideal. I don't think we
should/can accept something into the Qpid code base when it hasn't even been attempted to
use it with a product that is actually Apache Licence compatible. I think this really needs
to be able to work fully out the box with something like Geronimo, with additional rather
than sole configuration supplied for JBoss AS.
> > 
> > It probably gets round the letter of the licence, since although the example actually
imports some JBoss annotations it probably gets away with it because it doesn't look to be
built by default (correct?), whereas the adapter itself either has the bits turned off that
are JBoss dependent and/or uses reflection to avoid the imports. That doesn't really seem
to change the fact you don't appear to be able to use all of the 'optional' features without
JBoss though, which is probably still in violation of the spirit of the licence.
> > 
> > It still appears that there is absolutely zero test coverage for this? We keep talking
about trying to improve our test situation as a project, but particularly in the Java tree,
and additions like this are not helpful.
> > 
> > The code continues to violate the field naming convention we use on the Java components
( https://cwiki.apache.org/qpid/java-coding-standards.html ), although in contrast to earlier
versions this diff actually has a mixture of field naming conventions, making it inconsistent
as well.

A quick answer to a couple of things -

The adapter has actually been used with multiple application servers including JBoss and Websphere.
Also I believe that the TCK testing actually uses Sun/Oracles reference app server (I didn't
do the TCK testing myself).

It's not at all clear to me why the licensing of the App server matters for this code as the
resource adapter should work for all. So you're only talking about the default configuration,
why shouldn't the default configuration be JBoss? Its license is LGPL which is compatible
with the Apache License, but I'm not even sure that this is relevant.

I thought we hashed out the "testing" issue before on the dev list - in short there is little
coherent/useful that you could add to unit tests for the JCA as it's nearly all straight delegation
to the JMS client. And including an appserver into our java tree just to do unit testing seems
fantastic overhead. There appears to be little in between. The example code provides a reasonable
smoke test (given that you've installed an app server) which I believe is as good as any unit
tests would be.


> On 2011-07-15 15:40:16, Robbie Gemmell wrote:
> > /trunk/qpid/java/eclipse-projects/.gitignore, lines 1-2
> > <https://reviews.apache.org/r/441/diff/4/?file=25573#file25573line1>
> >
> >     Do we need an 'empty' directory in the repository for this?
> >     
> >     We have plenty of directories in the Java tree as it is without adding redundant
(for me at least) ones. Could this instead be added to the existing global .gitignore, or
a new one be added in the Java directory to ignore the subdirectory and its contents for those
that actually wish to create it?

Oops, that was meant to be removed, I'll get rid of it.


> On 2011-07-15 15:40:16, Robbie Gemmell wrote:
> > /trunk/qpid/java/jca/example/build.xml, line 41
> > <https://reviews.apache.org/r/441/diff/4/?file=25579#file25579line41>
> >
> >     Out of date hard coded value. Can we use the existing variables to keep this
up to date?

Good idea - I'll look at that


> On 2011-07-15 15:40:16, Robbie Gemmell wrote:
> > /trunk/qpid/java/jca/example/build.xml, lines 208-211
> > <https://reviews.apache.org/r/441/diff/4/?file=25579#file25579line208>
> >
> >     Out of date hard coded values. (project version as before, and we also just
upgraded to mina 1.1.7)

Yes I noticed the mina upgrade - that's good as it removes the backport dependency


> On 2011-07-15 15:40:16, Robbie Gemmell wrote:
> > /trunk/qpid/java/jca/example/src/qpid/jca/example/web/QpidTestServlet.java, lines
42-43
> > <https://reviews.apache.org/r/441/diff/4/?file=25590#file25590line42>
> >
> >     There are tab characters instead of spaces in several places within this file.

Yeah I noticed there are some remaining space problems in the code - I'll try to get rid of
them.


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/441/#review1077
-----------------------------------------------------------


On 2011-07-14 22:39:37, Andrew Stitcher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/441/
> -----------------------------------------------------------
> 
> (Updated 2011-07-14 22:39:37)
> 
> 
> Review request for qpid, Gordon Sim, Andrew Kennedy, Robbie Gemmell, rajith attapattu,
and Weston Price.
> 
> 
> Summary
> -------
> 
> Review for a qpid JCA resource adapter.
> 
> So far no build infrastructure is included.
> 
> I'd also like an opinion as to whether java/jca is the appropriate name for this (I'm
thinking perhaps java/ra would be more usual).
> 
> Any and all comments welcome.
> 
> 
> This addresses bug QPID-3044.
>     https://issues.apache.org/jira/browse/QPID-3044
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/java/lib/geronimo-j2ee-connector_1.5_spec-2.0.0.jar UNKNOWN 
>   /trunk/qpid/java/lib/geronimo-jta_1.1_spec-1.1.1.jar UNKNOWN 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/inflow/QpidMessageHandler.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/tm/JBossTransactionManagerLocator.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/resources/META-INF/ra.xml PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/inflow/QpidActivationSpec.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/admin/QpidConnectionFactoryProxy.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/admin/QpidDestinationProxy.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/admin/QpidQueue.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/admin/QpidTopic.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/inflow/QpidActivation.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/admin/AdminObjectFactory.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/admin/QpidBindingURL.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/Util.java PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidResourceAdapter.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAStreamMessage.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRATextMessage.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRATopicPublisher.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRATopicSubscriber.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAXAResource.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRASessionFactory.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRASessionFactoryImpl.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAQueueBrowser.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAQueueReceiver.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAQueueSender.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRASession.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAObjectMessage.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAProperties.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAMetaData.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAMessageProducer.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAMessageListener.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAMessageConsumer.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAMessage.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAMapMessage.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAManagedConnectionFactory.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAManagedConnection.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAMCFProperties.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRACredential.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAException.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRALocalTransaction.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAConnectionManager.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAConnectionMetaData.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAConnectionRequestInfo.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAConnectionFactory.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRAConnectionFactoryImpl.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/QpidRABytesMessage.java PRE-CREATION

>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/ConnectionFactoryProperties.java
PRE-CREATION 
>   /trunk/qpid/java/jca/src/main/java/org/apache/qpid/ra/ConnectionFactoryObjectFactory.java
PRE-CREATION 
>   /trunk/qpid/java/jca/example/src/qpid/jca/example/web/QpidTestServlet.java PRE-CREATION

>   /trunk/qpid/java/jca/qpid-jca-ds.xml PRE-CREATION 
>   /trunk/qpid/java/jca/example/conf/application.xml PRE-CREATION 
>   /trunk/qpid/java/jca/example/conf/qpid-jca-ds.xml PRE-CREATION 
>   /trunk/qpid/java/jca/example/conf/web.xml PRE-CREATION 
>   /trunk/qpid/java/jca/example/src/qpid/jca/example/QpidClient.java PRE-CREATION 
>   /trunk/qpid/java/jca/example/src/qpid/jca/example/QpidStandaloneClient.java PRE-CREATION

>   /trunk/qpid/java/jca/example/src/qpid/jca/example/ejb/QpidListener.java PRE-CREATION

>   /trunk/qpid/java/jca/example/src/qpid/jca/example/ejb/QpidTest.java PRE-CREATION 
>   /trunk/qpid/java/jca/example/src/qpid/jca/example/ejb/QpidTestBean.java PRE-CREATION

>   /trunk/qpid/java/jca/example/src/qpid/jca/example/ejb/QpidTestLocal.java PRE-CREATION

>   /trunk/qpid/java/jca/example/src/qpid/jca/example/ejb/QpidTestRemote.java PRE-CREATION

>   /trunk/qpid/java/build.deps 1070497 
>   /trunk/qpid/java/build.xml 1070497 
>   /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQQueueBrowser.java 1070497

>   /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/TopicPublisherAdapter.java
1070497 
>   /trunk/qpid/java/eclipse-projects/.gitignore PRE-CREATION 
>   /trunk/qpid/java/jca/README-JBOSS.txt PRE-CREATION 
>   /trunk/qpid/java/jca/README.txt PRE-CREATION 
>   /trunk/qpid/java/jca/build.xml PRE-CREATION 
>   /trunk/qpid/java/jca/example/.gitignore PRE-CREATION 
>   /trunk/qpid/java/jca/example/README PRE-CREATION 
>   /trunk/qpid/java/jca/example/build.xml PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/441/diff
> 
> 
> Testing
> -------
> 
> This code is now substantially the same as tested by Red Hat, passes the TCK.
> 
> 
> Thanks,
> 
> Andrew
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message