> On 2011-02-18 07:55:41, Robbie Gemmell wrote: > > If the code is based on something written by another author originally > > (most of the files crediting other authors suggests this is this case?) I > > think it should be commited in its original state first so that we can > > verify the base usage is legitimate and also track the changes made to > > transform it. Also, if the code was developed by others originally, should > > it have the ASF attributed version of the Apache Licence header? See > > http://www.apache.org/legal/src-headers.html. > > > > The code doesn't confirm to some parts of the general code style used by > > (most of) the Java codebase (described at > > https://cwiki.apache.org/qpid/java-coding-standards.html), likely due to > > its seeming origin but still worth noting. Specifically the instance > > variable naming doesn't use prefixed underscores to differentiate them from > > local variable names (helps identifying/preventing variable shadowing). > > This is > > > > The style used for the logging is an example of where changes should be > > made, eg: > > > > if (ConnectionFactoryProperties.trace) > > { > > ConnectionFactoryProperties.log.trace("getDefaultUsername()"); > > } > > > > ConnectionFactoryProperties.trace and ConnectionFactoryProperties.log.trace > > are a lot longer and less clear (initially I thought it was doing something > > worse and accessing public variables in another class) than just using > > _trace and _log.trace if following the instance variable style mentioned > > above. Also, ConnectionFactoryProperties.trace as noted above is defined > > statically which means it can't be manipulated to enable trace logging > > while running, although it may be (appreciably?) faster than doing the > > normally used alternative: if(_log.isTraceEnabled()). > > > > This would instead give: > > if (_log.isTraceEnabled()) > > { > > _log.trace("getDefaultUsername()"); > > } > > > > Also, in that particular class (ConnectionFactoryProperties) the Logger is > > incorrectly defined as using the log hierarchy for > > QpidRAMCFProperties.class: > > private static final Logger log = > > LoggerFactory.getLogger(QpidRAMCFProperties.class);
The code is based on the hornetq adapter and I have permission to donate this version to apache qpid (I work for Red Hat, the copyright owner). I believe we have applied the appropriate header based on your link. Apologies for the coding style but I was asked to get a JCA adapter working and passing the TCK as quickly as possible, no attempt has been made to integrate it into the build nor reformat according to coding styles. There are better folk than I that can handle this part. Apologies again for the logging, this is an artifact of the original code. I will tidy this up and correct the definitions. - kevinconner ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/432/#review232 ----------------------------------------------------------- On 2011-02-17 12:17:18, Andrew Stitcher wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/432/ > ----------------------------------------------------------- > > (Updated 2011-02-17 12:17:18) > > > Review request for qpid. > > > Summary > ------- > > This is the first review candidate 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/jca/src/main/java/org/apache/qpid/ra/ConnectionFactoryObjectFactory.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/QpidRABytesMessage.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/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/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/QpidRAMCFProperties.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/QpidRAManagedConnectionFactory.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/QpidRAMessage.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/QpidRAMessageListener.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/QpidRAMetaData.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/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/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/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/QpidResourceAdapter.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/inflow/QpidActivation.java > 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/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/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 > > Diff: https://reviews.apache.org/r/432/diff > > > Testing > ------- > > > Thanks, > > Andrew > >
